Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memoize array of drop getter method names #8421

Merged
merged 8 commits into from
Oct 16, 2020

Conversation

ashmaroli
Copy link
Member

  • This is an ⚡ optimization change.
  • I've added tests.

Summary

Instead of stringifying array of drop-instance-methods for every drop instance, compute them once per drop class and reuse the resulting array of strings.

@ashmaroli ashmaroli marked this pull request as ready for review October 7, 2020 18:18
@ashmaroli ashmaroli requested a review from a team October 7, 2020 18:19
@ashmaroli ashmaroli added memory-optimization ⚡ Reduced memory usage for the work done optimization ⚡ labels Oct 8, 2020
@ashmaroli
Copy link
Member Author

Third-party Repo Memory Profile Summary

--- master https://github.com/jekyll/jekyll/runs/1241364408
+++ PR     https://github.com/jekyll/jekyll/runs/1263735861

- Total allocated: 224.63 MB (1271956 objects)
- Total retained:  40.74 MB (130038 objects)
+ Total allocated: 204.96 MB (1193919 objects)
+ Total retained:  40.77 MB (130586 objects)

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tests pass, we can see a slight faster build on the third-party sample repository. 👏

@DirtyF DirtyF added this to the 4.2 milestone Oct 16, 2020
@ashmaroli
Copy link
Member Author

Thank you Frank :)
@jekyllbot: merge +minor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants