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

Reduce allocations by using #each_with_object #7758

Merged
merged 4 commits into from Aug 4, 2019

Conversation

ashmaroli
Copy link
Member

Summary

The loop within config.reduce({}) { |hsh, (k, v)| hsh.merge(k.to_s => v) } is going to run for each
key-value pair in config and on each iteration, hsh.merge is going to allocate a new Hash object.

Therefore, if a config file has 20 top-level key-value pairs, this method would've allocated at least 20 Hash objects (of increasing size)..
This can be bad for sites that have numerous top-level config keys (e.g. forks of minimal-mistakes theme repo)

@ashmaroli ashmaroli requested a review from a team July 22, 2019 18:13
lib/jekyll/configuration.rb Outdated Show resolved Hide resolved
@ashmaroli ashmaroli changed the title Reduce Hash allocations by using Hash#merge! Reduce allocations by using #each_with_object Jul 22, 2019
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Functional programming can be fun, but isn’t always efficient in this wild world of Ruby we’re living in. 😄 Thanks for fixing this up!

@ashmaroli
Copy link
Member Author

ashmaroli commented Jul 23, 2019

@parkr Sorry, I found one more instance of Hash[array.map { |k| [k, foo(k)] }] after you approved the PR.

@ashmaroli
Copy link
Member Author

The gains aren't much, but probably non-trivial for much larger sites:

Profiler Summary

--- master branch https://travis-ci.org/jekyll/jekyll/jobs/561428064
+++ PR branch     https://travis-ci.org/jekyll/jekyll/jobs/562630268

- Total allocated: 479.11 MB (4483645 objects)
- Total retained:  18.54 MB (96252 objects)
+ Total allocated: 479.06 MB (4482911 objects)
+ Total retained:  18.54 MB (96251 objects)

@DirtyF
Copy link
Member

DirtyF commented Aug 4, 2019

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 65f8831 into jekyll:master Aug 4, 2019
jekyllbot added a commit that referenced this pull request Aug 4, 2019
@ashmaroli ashmaroli deleted the reduce-hash-allocations branch August 5, 2019 04:13
@jekyll jekyll locked and limited conversation to collaborators Aug 4, 2020
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

5 participants