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

Optimize Page#dir with a private method #8489

Merged
merged 2 commits into from
Dec 2, 2020

Conversation

ashmaroli
Copy link
Member

  • This is an ⚡ optimization change

Summary

In the same boat as #8485, because Jekyll::Page#to_liquid isn't memoized, Jekyll::Page#dir get called for every call to #to_liquid.

Unfortunately, #dir can't be directly memoized either because @dir is already defined in Jekyll::Page#initialize and has different semantics in comparison to the result from calling #dir. Using a new instance variable name is risky because Jekyll::Page is a popular ancestor for Generator based plugins.

Therefore, move the allocating logic into a new private and memoized method.

@ashmaroli ashmaroli requested a review from DirtyF December 2, 2020 13:16
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, don't see any build time improvement in the test suite though.

@ashmaroli
Copy link
Member Author

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit c80ccbe into jekyll:master Dec 2, 2020
jekyllbot added a commit that referenced this pull request Dec 2, 2020
@ashmaroli ashmaroli deleted the optimize-page-dir branch December 2, 2020 17:27
@ashmaroli ashmaroli removed the bug label Dec 2, 2020
github-actions bot pushed a commit that referenced this pull request Dec 2, 2020
Ashwin Maroli: Optimize Page#dir with a private method (#8489)

Merge pull request 8489
github-actions bot pushed a commit to tigefa4u/jekyll that referenced this pull request Dec 2, 2020
jekyllbot: Update history to reflect merge of jekyll#8489 [ci skip]
github-actions bot pushed a commit to patilswapnilv/jekyll that referenced this pull request Dec 2, 2020
jekyllbot: Update history to reflect merge of jekyll#8489 [ci skip]
github-actions bot pushed a commit to patilswapnilv/jekyll that referenced this pull request Dec 2, 2020
jekyllbot: Update history to reflect merge of jekyll#8489 [ci skip]
github-actions bot pushed a commit to chauncey-garrett/jekyll that referenced this pull request Dec 2, 2020
jekyllbot: Update history to reflect merge of jekyll#8489 [ci skip]
github-actions bot pushed a commit to chauncey-garrett/jekyll that referenced this pull request Dec 2, 2020
jekyllbot: Update history to reflect merge of jekyll#8489 [ci skip]
@jekyll jekyll locked and limited conversation to collaborators Dec 2, 2021
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