Lazily check incomplete lockfile to improve performance#5546
Lazily check incomplete lockfile to improve performance#5546deivid-rodriguez merged 7 commits intomasterfrom
Conversation
deivid-rodriguez
force-pushed
the
lazily-check-incomplete-lockfile
branch
from
f10a6e8 to
ee0c989
Compare
|
I also tried this against the big Gemfile published by @technicalpickles at #5545, and I only get a 2% speed. Quite disappointing but better than nothing I guess. |
|
I tested on our project's Gemfile and got an 8% improvement 😁 I was curious how it stacks with the other PRs we have going, and seems they end up making things worse 🙊 edit: corrected observation and data in the last block after I realized I was running against the wrong branch |
|
@technicalpickles Good news that 8% speed up. I don't understand though how the other patches could ever harm performance, and it's not what I observed. #5537 needs to be adapted to this PR to use diff --git a/bundler/lib/bundler/spec_set.rb b/bundler/lib/bundler/spec_set.rb
index f0f9d093a0..b05084bd4c 100644
--- a/bundler/lib/bundler/spec_set.rb
+++ b/bundler/lib/bundler/spec_set.rb
@@ -12,15 +12,15 @@ def initialize(specs)
end
def for(dependencies, check = false, platforms = [nil])
- handled = ["bundler"].product(platforms)
+ handled = ["bundler"].product(platforms).map {|k| [k, true] }.to_h
deps = dependencies.map(&:name).product(platforms)
specs = []
loop do
break unless dep = deps.shift
- next if handled.include?(dep)
+ next if handled.key?(dep)
- handled << dep
+ handled[dep] = true
specs_for_dep = spec_for_dependency(*dep)
if specs_for_dep.any?With that on top, I get the following results on Ruby 2.7.5 (this PR + using a hash to track handled items in Something very interesting is that I get much bigger speed up on Ruby 3.1.2 So another interesting finding here is that Ruby 2.7 is significantly faster for us than Ruby 3.1. |
|
And for completeness, Ruby 3.0 behaves exactly the same as Ruby 3.1, so it seems like some change in the 2.7 -> 3.0 transition. |
|
@technicalpickles Are you planning to dig further on why you observed different results when stacking PRs, or did you already find an explanation for it? |
|
It took me a bit to recover from RailsConf 😅 I ran it against our repo with master, #5537 , #5546 , and then those two combined (. I only tested Ruby 2.7.5, since we aren't on 3.0 yet. For big-gemfile: For our app: Parsing those out to a table...
If I were to draw a conclusion from that, lazily-check-incomplete-lockfile is slightly slower (6.2%) on the big-gemfile I created, but significantly slower on our app (47%). specset-for-hash-lookup-of-handled-deps was the fastest branch, with 7.5% improvement for big-gemfile, and 58% improvement for our app. Combining the approaches was very slightly slower than using specset-for-hash-lookup-of-handled-deps on its own. I wanted to call out that I'm specifically calling testing this PS: I tried to do a few PPS: I accidentally hit command-enter and commented before finishing this. |
|
No worries @technicalpickles! I'm super confused with your results, since now they seem to contradict your initial testing where you reported an 8% improvement, no? To clarify how I test this. I edit the Then I let hyperfine compare the different branches by setting
|
|
Let me try clearing out my various dev gems, and try again. I also realized that the run.rb may trigger actually resolving the dependencies because it displays a count of gems. I'll try again with the simpler one liner you used. |
deivid-rodriguez
force-pushed
the
lazily-check-incomplete-lockfile
branch
2 times, most recently
from
8906a1a to
31adc0c
Compare
|
@technicalpickles Did you find time to try this? I'm really curious to clarify this and I'd like to ship this improvement :) |
|
Alright, something we can do while you try this out is go ahead and ship #5537. since that's the biggest speed up here and clearly can't ever make things worse. Once we have that, I can rebase this PR and we can evaluate its performance more easily. I'll work on that! |
deivid-rodriguez
force-pushed
the
lazily-check-incomplete-lockfile
branch
from
31adc0c to
203136f
Compare
deivid-rodriguez
force-pushed
the
lazily-check-incomplete-lockfile
branch
from
203136f to
bc04bb6
Compare
|
This PR is now rebased and ready to be tried out :) |
deivid-rodriguez
force-pushed
the
lazily-check-incomplete-lockfile
branch
2 times, most recently
from
0ba2c9b to
3c375fa
Compare
|
These are updated number on my computer after other speedups were released So only 2-3% speed up now. But I'm working on a different approach where the speed up goes up to 5-6% and that might make this one unnecessary. I'll propose it separately. |
|
And here is the alternative PR: #5695. By the way @technicalpickles, I think I understood why you were getting different results. Are you running your tests on Linux by any chance? Since the |
deivid-rodriguez
force-pushed
the
lazily-check-incomplete-lockfile
branch
2 times, most recently
from
211e608 to
744bd5e
Compare
|
Good news, after rebasing this on top of other improvements, it still provides a 5-6% speed up on the big gemfile we've been using for testing! 🎉 I'm setting this as ready and I'll be merging in a couple of days if I don't get reviews. |
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.
deivid-rodriguez
force-pushed
the
lazily-check-incomplete-lockfile
branch
from
744bd5e to
8ce54de
Compare
deivid-rodriguez
force-pushed
the
lazily-check-incomplete-lockfile
branch
from
8ce54de to
5281e51
Compare
Lazily check incomplete lockfile to improve performance (cherry picked from commit f847e60)
2 participants
What was the end-user or developer problem that led to this PR?
We have some code that checks whether the lockfile has incomplete specs for the current platform, i.e., when even if the current platform is locked, the lockfile is missing some specs for it.
I think this check was introduced due to some bug in Bundler that generated some incomplete lockfiles, but it should be a very edge case. However, we check this every time
bundler/setupis required, so all usages have to pay the cost of trying to gracefully handle this edge case.What is your fix for the problem, implemented in this PR?
Checking this edge case involves actually resolving the locked specs for the current platform, which is something we need to do later anyways. So my approach is to assume this edge case does not happen, and when going ahead and materializing the actual set of specifications, check whether it actually happened. If that's the case, then go ahead and re-resolve.
This should reduce the number of times
bundler/setupcallsBundler::SpecSet#forfrom 3 to 2.The benefit on performance is unfortunately more moderate than I was expecting, about 1% on a fresh new rails application and about 2% on
rails/railsrepository Gemfile. But I would expect it to be better for bigger Gemfiles.Make sure the following tasks are checked