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 `Site#site_data` #6809

Merged
merged 2 commits into from Mar 14, 2018

Conversation

Projects
None yet
5 participants
@ashmaroli
Member

ashmaroli commented Feb 26, 2018

  • Site#site_data is called twice per page / document when the popular plugin jekyll-seo-tag is used
  • jekyll-seo-tag is loaded by default via recent releases of Minima
  • IMO, its better addressing this here instead of patching per plugin(s)

@DirtyF DirtyF requested review from jekyll/stability Feb 26, 2018

@@ -253,7 +253,7 @@ def categories
#
# Returns the Hash to be hooked to site.data.
def site_data
config["data"] || data
@site_data ||= config["data"] || data

This comment has been minimized.

@parkr

parkr Feb 27, 2018

Member

If you want to memoize the ||, then it'd be: @site_data ||= (config["data"] || data). By default, this is equivalent to (@site_data ||= config["data"]) || data.

This comment has been minimized.

@parkr

parkr Feb 27, 2018

Member

This @site_data should also be reset in reset!

This comment has been minimized.

@ashmaroli

ashmaroli Feb 27, 2018

Member

Regarding the use of parenthesis, I did not know, this would be interpreted that way.. Thanks!

This comment has been minimized.

@parkr

parkr Feb 27, 2018

Member

@ashmaroli You might enjoy http://tmm1.net/ruby21-profiling/ which has an example of where parentheses went awry!

This comment has been minimized.

@ashmaroli

ashmaroli Feb 27, 2018

Member

Thanks. I'll check it out 😃

@DirtyF

DirtyF approved these changes Feb 27, 2018

@parkr

parkr approved these changes Feb 27, 2018

@DirtyF DirtyF added this to the v3.8.0 milestone Mar 1, 2018

@oe

This comment has been minimized.

Member

oe commented Mar 14, 2018

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 55d64c7 into jekyll:master Mar 14, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ashmaroli ashmaroli deleted the ashmaroli:memoize-site-data branch Mar 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment