Speed up complex dependency resolves by creating DepProxy factory and cache #4216
Conversation
|
Oh wow, we definitely want this it's great 🎉! I'll have a detailed look tomorrow 👍. |
|
Great investigation work here, thanks for the PR! |
lukaso
force-pushed
the
depproxy-factory
branch
from
15b0e0b
to
55caac9
Compare
|
My pleasure. Thanks. |
|
This additional change looks like it took this case from 1m32s down to 10s on my laptop. The |
There was a problem hiding this comment.
Hei, this is really awesome, thanks soooooo much! ❤️
I made some small comments, but this is going in, soon! I think you can start proposing the molinillo changes upstream 😃.
Also, rubygems has a separate copy of molinillo, maybe it's worth updating that copy too.
lukaso
force-pushed
the
depproxy-factory
branch
from
d72cd84
to
efc074e
Compare
|
Ok, I've been working on finishing this up and it's almost ready. I'll force-push to this branch soon. Current performance on my machine with the problematic Gemfile is ~3 seconds with the molinillo patch, and ~10 seconds without it 🎉. |
|
I'll take a look once ready. |
|
Great! I created a PR to Molinillo: CocoaPods/Molinillo#146. If it passes Molinillo's specs I think we can ship it. |
Fantastic news. |
deivid-rodriguez
force-pushed
the
depproxy-factory
branch
2 times, most recently
from
0222795
to
53a6167
Compare
|
Yay, it's great! I added some tweaks to get this ready. Suming them up:
I think that's it. @simi You can have a look now! |
|
Because it’s mapping from an infinite dimensional space (any possible array) to a fixed-width integer.
On Jan 7, 2021, at 12:58 PM, David Rodríguez <notifications@github.com> wrote:
@deivid-rodriguez commented on this pull request.
In bundler/lib/bundler/dep_proxy.rb:
@@ -4,19 +4,18 @@ module Bundler
class DepProxy
attr_reader :__platform, :dep
+ @proxies = {}
+
+ def self.get_proxy(dep, platform)
+ @proxies[[dep, platform].hash] ||= new(dep, platform).freeze
I think we are confusing the #hash (method) ruby uses to determine the key used under the hood when using objects as keys with the hash (function) used by the hash (data structure) implementation to map keys to buckets. The hash (function) can certainly have collisions that should be handled by the implementation, but I don't see why the #hash method on an Array should have collisions and not be unique to the array contents.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
That makes sense @segiddins, but won't ruby take care of collisions when generating hashes? If you implement your own In any case, even if we were to really fix this I'm not sure how to go about it, since using an array as the hash key I would imagine So, to sum up, I don't think this is a problem in practice so I'm fine with whatever alternative you or @indirect think is safest. I'll just update this to use the |
The `~>` operator is special because what's on the right side of it doesn't have the same properties as a `Gem::Version`. For example, `~> 2.0.0` is not the same as `~> 2.0` even if `2.0` and `2.0.0` are equivalent as `Gem::Version`'s.
This means that all equal `DepProxy` objects will be the same object as they are deduped on create. This then in turn allows `Bundler::Molinillo::Resolver#build_details_for_unwind` function to use `Array#-` to find relevant unused unwinds which is about 5 times faster than `Array#&` was before. This dramatically improves performance for some use cases as this function was where most of the time in the resolution was being consumed.
This is a hot code path during resolution and `Array#-` performs much better than `Array#&`. With the following sample `Gemfile`: ```ruby source 'https://rubygems.org' gem "actionmailer" gem "mongoid", ">= 0.10.2" ``` It reduces `bundle lock` time from 9.5s to 2.8s on my machine. Co-authored-by: Lukas Oberhuber <lukas.oberhuber@simplybusiness.co.uk>
deivid-rodriguez
force-pushed
the
depproxy-factory
branch
from
53a6167
to
fb284ea
Compare
|
@simi In case it helps with your review (assuming you're interested in reviewing the part about hash collisions specifically), I made some research on ruby sources: This defines If my interpretation of the C code is correct (which is assuming too much already 😅), that would mean that all alternatives proposed here are equally safe (or unsafe, although I would say they are all safe). |
I think the |
|
Still need to investigate it more, but as of today, the It seems that some recent gem releases made the number of steps in that resolution go from ~1000 to ~6000. With this PR, resolution is at least workable taking ~ 1minute. All that to say, we should merge this. I think it has gone through enough scrutiny already. If nobody beats me to it, I'll be merging this later today, we can address any late maintainer feedback post-merge. |
Speed up complex dependency resolves by creating DepProxy factory and cache (cherry picked from commit 9b50ef6)
What was the end-user or developer problem that led to this PR?
Running some (relatively complex) resolution cases can lead to extremely poor performance of
Resolving dependencies. Here is a trivial case to reproduce this which runs in 22 minutes on my laptop.Gemfile
The same problem has been seen in the real world at Simply Business where we have repeatedly run into resolution taking excessively long or even hanging.
What is your fix for the problem, implemented in this PR?
Solution
The solution in this PR takes the above case from 22 minutes to 1 minute 32 seconds (on my laptop).
On other runs with different gem files, I've seen improvements including a 30% improvement and a 5 x improvement.
For the general case, there is no performance implications.
The basic solution (I will describe the diagnostics below) that led me to this requires two things:
DepProxyby creating a factory method and comparing requests for newDepProxys to existing proxies (which creates a high cache hit rate). The using the hash of the objects to cache. This also allows comparison of two exactly the sameDepProxys to use the inbuiltObject#==which is very fast. This does mean that the objects are now shared. If they were immutable before, this would be totally fine, but even if they aren't, it appears not to cause problems.build_details_for_unwindinresolution.rbwith an implementation forrelevant_unused_unwindsthat usesArray#-which takes advantage of object comparison rather thanArray#&which usesArray#hashand in this case is substantially slower. Without the Molinillo change, theDepProxyfactory is performance neutral.build_details_for_unwindand specificallyArray#&were where most of the time in the cases above were spent, as the analysis will demonstrate.Note on the PR
I present the full PR with vendored changes, assuming if you would like to move forward I would submit a PR to
Molinillo.Analysis
The added
slow_perf_spec.rbcan run against this branch ormaster. On this branch it runs in:On
masterit runs:This gives a savings of two minutes or ~ 30%.
Here is the output of the old and new algorithms:
DepProxy branch
Master branch
The results of running
ruby-profonmasterand on the new branchOriginal implementation
With adding a DepProxy factory
Other interesting findings
Interestingly, by switching the
mongoidversion from>=0.10.2to>=0.10.3, steps to resolve go from 6264 down to 827 due to picking a different starting requirment (although I haven't analysed the algorithm to understand why the change is triggered). Red or<version is>=0.10.3and Green or>is>=0.10.2. There is also a dramatic slowdown in dependency resolution between the two options, due to the very slow algorithm (even with the fixes proposed here) related to looking for previous unwind states.Make sure the following tasks are checked