Lazily check incomplete lockfile+specset for hash lookup of handled deps#5582
Closed
Conversation
So that it deals with [name, platform] tuples consistently instead of dealing with `Gem::Dependency` or `Bundler::DepProxy` instances inconsistently.
The resolve might be against locally available gems, or remote gems, depending on the situation.
This is a very weird edge case, not all Bundler invokations should pay its cost. Instead, assume it's not going to happen, detect the situation at materialization time, and re-resolve.
Since the improvement to lazily check whether the lockfile has missing platform specific dependencies, special handling on "bundler" inside `Bundler::SpecSet#for` is no longer covered by specs, because even in the case where missing platform specific dependencies are found, we still show the "Found no changes" message initially. This extra assertion makes the code covered again.
…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
lazily-check-incomplete-lockfile
branch
2 times, most recently
from
8906a1a to
31adc0c
Compare
deivid-rodriguez
force-pushed
the
lazily-check-incomplete-lockfile
branch
from
31adc0c to
203136f
Compare
Contributor
|
Since we have merged #5537, there should be no need to combine both PRs anymore since you can now compare directly against the master branch :) |
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.
Test PR to combine #5546 and #5537 for benchmarking
What was the end-user or developer problem that led to this PR?
What is your fix for the problem, implemented in this PR?
Make sure the following tasks are checked