Skip to content

Memoize some #hash calls#5532

Closed
ngan wants to merge 3 commits intoruby:masterfrom
ngan:memoize-identity-hash
Closed

Memoize some #hash calls#5532
ngan wants to merge 3 commits intoruby:masterfrom
ngan:memoize-identity-hash

Conversation

@ngan
Copy link
Copy Markdown
Contributor

@ngan ngan commented May 12, 2022

What was the end-user or developer problem that led to this PR?

require 'bundler/setup is slow for large Gemfiles (eg 500+ gems)

What is your fix for the problem, implemented in this PR?

Memoize some #hash implementations since they don't change and we do it a lot for large gem files.

Before

➜  time ruby -e 'require "bundler/setup"' 
1.49s user 0.11s system 97% cpu 1.645 total
➜  time ruby -e 'require "bundler/setup"'
1.50s user 0.12s system 96% cpu 1.668 total
➜ time ruby -e 'require "bundler/setup"'
1.50s user 0.12s system 96% cpu 1.678 total

After

➜  time ruby -e 'require "bundler/setup"'
0.95s user 0.12s system 94% cpu 1.136 total
➜  time ruby -e 'require "bundler/setup"'
0.95s user 0.12s system 94% cpu 1.125 total
➜ time ruby -e 'require "bundler/setup"'
0.96s user 0.12s system 95% cpu 1.128 total

About 30% less time.

Make sure the following tasks are checked

@ngan
Copy link
Copy Markdown
Contributor Author

ngan commented May 12, 2022

@deivid-rodriguez here's a small change for a pretty decent performance improvement. I tried memoizing more things in rubygems but decided not to since I'm unsure how useful it'd be.

@ngan ngan changed the title Memoize all #hash calls Memoize some #hash calls May 13, 2022
Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

This looks very good, thanks for this. A 30% improvement for a one line change feels pretty great.

Do any of the other changes have any effect at all on your application? I'm asking because it might be best to just change "the money maker" here. Other changes involve vendored code from Molinillo and code monkey patched from RubyGems, so we probably should not change those without changing the upstream too. Also, I'm not sure changing sources has any effect given that the source list is usually short and sources are not compared often.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

I tried this patch on rails/rails Gemfile and it leads to a pretty nice 8% bundler/setup time improvement! Also verified that only the change in LazySpecification#hash contributes to that improvement.

@ngan
Copy link
Copy Markdown
Contributor Author

ngan commented May 13, 2022

Yeah, I got a little carried away. Happy to just memorize the lazy spec only. It’s late for me so I won’t be able to get to it until tomorrow. Feel free to modify this pr and land! It’s a very simple change. Otherwise I can get to it tomorrow.

@ngan
Copy link
Copy Markdown
Contributor Author

ngan commented May 13, 2022

Closing this out in favor of #5533

@ngan ngan closed this May 13, 2022
@deivid-rodriguez
Copy link
Copy Markdown
Contributor

So, we can confirm that #5533 does beat the 30% improvement of this PR in your application, and that this patch no longer has an effect in performance after #5533?

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

Thank you so much for your investigation and patch, by the way!! ❤️ I obviously credited you on #5533 since I wouldn't have found the culprit without this hint!

@ngan
Copy link
Copy Markdown
Contributor Author

ngan commented May 13, 2022

No problem. @technicalpickles and I will find some more time and more hints for you 😄

There’s actually a problem with the latest released version though so we haven’t been able to upgrade. We’re unable to do a bundle install from scratch. I’ll take a closer look today to see if it’s something on our end or it’s because of the last speed up changes.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

Ouch, yeah, let me know what you find! :)

@ngan
Copy link
Copy Markdown
Contributor Author

ngan commented May 13, 2022

Ouch, yeah, let me know what you find! :)

@deivid-rodriguez I started an issue: #5538

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants