Improve performance of Bundler::SpecSet#for by using hash lookup of handled deps#5537
Merged
deivid-rodriguez merged 1 commit intoruby:masterfrom Jun 19, 2022
Conversation
Contributor
4 tasks
deivid-rodriguez
force-pushed
the
specset-for-hash-lookup-of-handled-deps
branch
from
7d3f4a8 to
7578bcd
Compare
Contributor
|
This looks great to me, I run it against the So an extra 4% speed up over current master branch! |
4 tasks
Contributor
Author
I had a chance to test this against https://github.com/technicalpickles/big-gemfile I cracked open a flamegraph using both the branches, and I'm seeing |
4 tasks
Contributor
|
I'm not seeing the same results with your sample repository 😮. I get the results that I would expect, namely, this PR improves performance by a lot, but once applied, #5534 and #5536 no longer have any effect. |
4 tasks
deivid-rodriguez
changed the title
Bundler::SpecSet#for by using hash lookup of handled deps
…andled deps
I was looking at (yet another) flamegraph in speedscope, and used the
'left hand heavy' and was shocked to realize that 0.5s of the 1.7s
is spent in DepProxy#name. This method _only_ delegates the name to an
underlying spec, so it's not complex at all.
It seems to be of how often this line ends up calling it:
next if handled.any?{|d| d.name == dep.name && (match_current_platform || d.__platform == dep.__platform) } || dep.name == "bundler"
The `handled` array is built up as dependencies are handled, so this get
slower as more dependencies are installed.
This change changes how `handled` is track. Instead of just an array, I've
tried using a Hash, with the key being a dep's name, and the value being
a list of deps with that name. This means it's constant time to find
the dependencies with the same name.
I saw a drop from 1.7s to 1.0s against master, and from 0.95s to 0.24s
when used with ruby#5533
deivid-rodriguez
force-pushed
the
specset-for-hash-lookup-of-handled-deps
branch
from
7578bcd to
844dac3
Compare
deivid-rodriguez
approved these changes
Jun 18, 2022
Contributor
|
Let's do this! |
deivid-rodriguez
added a commit
that referenced
this pull request
Jun 29, 2022
…p-of-handled-deps Improve performance of `Bundler::SpecSet#for` by using hash lookup of handled deps (cherry picked from commit 4b1b2b7)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was the end-user or developer problem that led to this PR?
Looking for improvements to runtime speed of
require 'bundler/setup'What is your fix for the problem, implemented in this PR?
I was looking at (yet another) flamegraph in speedscope, and used the
'left hand heavy' and was shocked to realize that 0.5s of the 1.7s
is spent in DepProxy#name. This method only delegates the name to an
underlying spec, so it's not complex at all.
It seems to be of how often this line ends up calling it:
The
handledarray is built up as dependencies are handled, so this getslower as more dependencies are installed.
This change changes how
handledis track. Instead of just an array, I'vetried using a Hash, with the key being a dep's name, and the value being
a list of deps with that name. This means it's constant time to find
the dependencies with the same name.
I saw a drop from 1.7s to 1.0s against master, and from 0.95s to 0.24s
when used with #5533
Make sure the following tasks are checked