diff options
author | The Bundler Bot <bot@bundler.io> | 2017-03-31 00:37:26 +0000 |
---|---|---|
committer | The Bundler Bot <bot@bundler.io> | 2017-03-31 00:37:26 +0000 |
commit | fa61ff501492b74cb11cff1fef82aa9038784511 (patch) | |
tree | 34a32e5d01f316f762bdf4a4e37e460fe2a54b0f | |
parent | ca5710e41dacefce8951f4992378d95b470882dc (diff) | |
parent | 6e7ed38d503de61a9b3721e06ff7d17bd6e07c11 (diff) | |
download | bundler-fa61ff501492b74cb11cff1fef82aa9038784511.tar.gz |
Auto merge of #5546 - jules2689:master, r=segiddins
Change `definition#converge_dependencies` comparison to an iterator for performance
What this addresses
---
Follow up for #5539
Previously, a proc was used to extract name and requirements from dependencies. This was
done twice, `2*O(n)`, and compared as `Set` objects.
This was identified as slow:
![image](https://cloud.githubusercontent.com/assets/3074765/24470432/e87f5324-148c-11e7-81c6-3bb4216c4e83.png)
** Again, note that tracepoint slows the results in the chart down.
Without `TracePoint`, we run at about 13-15ms.
How this fixes it
---
Now that we use a hash for `@locked_deps`, we can
iterate one of the arrays (`@dependencies`) and compare against entries in `@locked_deps` with `O(1)`
access. This results in `O(n)` access.
The result is that `converge_dependencies`, which used to take 15ms, now takes about 2-3ms.
![image](https://cloud.githubusercontent.com/assets/3074765/24470454/f7db5638-148c-11e7-8005-1388c4bacea2.png)
** Again, note that tracepoint slows the results in the chart down.
Considerations
---
Equality between 2 dependencies, as defined [here for =~](https://docs.ruby-lang.org/en/2.4.0/Gem/Dependency.html), shows us that equality means:
> This dependency will match if the name matches the other's name, and other has only an equal version requirement that satisfies this dependency.
To make sure we do not regress, we need to check the name and the requirements. Looking at the source code, we can actually just compare the deps as that doesnt take into account `type`.
Running the added test from #5354, it passed. So these assumptions seem to hold.
cc @segiddins as you wrote #5354 and reviewed #5539
-rw-r--r-- | lib/bundler/definition.rb | 13 |
1 files changed, 11 insertions, 2 deletions
diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb index 5bff85332f..a4aebe95ed 100644 --- a/lib/bundler/definition.rb +++ b/lib/bundler/definition.rb @@ -695,8 +695,17 @@ module Bundler dep.platforms.concat(@platforms.map {|p| Dependency::REVERSE_PLATFORM_MAP[p] }.flatten(1)).uniq! end end - dependency_without_type = proc {|d| Gem::Dependency.new(d.name, *d.requirement.as_list) } - Set.new(@dependencies.map(&dependency_without_type)) != Set.new(@locked_deps.values.map(&dependency_without_type)) + + # We want to know if all match, but don't want to check all entries + # This means we need to return false if any dependency doesn't match + # the lock or doesn't exist in the lock. + @dependencies.any? do |dependency| + locked_dep = @locked_deps[dependency.name] + next true if locked_dep.nil? + # We already know the name matches from the hash lookup + # so we only need to check the requirement now + dependency.requirement != locked_dep.requirement + end end # Remove elements from the locked specs that are expired. This will most |