Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
drop hash allocations and optimize for success cases
We rarely care what the conflicts on a spec are, just whether or not
there are conflicts. This commit introduces an allocation-free method
that returns whether or not there are conflicts. The only case we
actually need to know the conflicts are in the case that an exception is
raised, and in that case we can do the work twice since the program will
exit anyway. This drops hash allocations.
Benchmark:
```ruby
require 'stackprof'
require 'allocation_tracer'
require 'rubygems/test_case'
require 'rubygems/ext'
require 'rubygems/specification'
class TestGemSpecification < Gem::TestCase
def test_find_in_unresolved_tree_is_not_exponentiental
save_loaded_features do
num_of_pkg = 7
num_of_version_per_pkg = 3
packages = (0..num_of_pkg).map do |pkgi|
(0..num_of_version_per_pkg).map do |pkg_version|
deps = Hash[(pkgi..num_of_pkg).map { |deppkgi| ["pkg#{deppkgi}", ">= 0"] }]
new_spec "pkg#{pkgi}", pkg_version.to_s, deps
end
end
base = new_spec "pkg_base", "1", {"pkg0" => ">= 0"}
Gem::Specification.reset
install_specs base,*packages.flatten
base.activate
require 'benchmark'
ObjectSpace::AllocationTracer.setup(%i{path line type})
r = ObjectSpace::AllocationTracer.trace do
assert_raises(LoadError) { require 'no_such_file_foo' }
end
r.sort_by { |k,v| v.first }.each do |k,v|
p k => v
end
p hash_alloc: ObjectSpace::AllocationTracer.allocated_count_table[:T_HASH]
p :TOTAL => ObjectSpace::AllocationTracer.allocated_count_table.values.inject(:+)
end
end
end
```
Before: 1562496 Hash allocations
After: 0 Hash allocations- Loading branch information
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any chance that you will be available to do things this amazing for Bundler, sometime in the future? Because this is amazing.
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do it for bundler too, but someone else identified this bottleneck. I'm actually just doing superficial perf improvements until we can figure out a better algorithm. :(
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like saving 5 million allocations is more than superficial, but okay! I'd love a better algorithm, too.😁
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If RubyGems is better at resolving why not have bundler use RubyGems for resolution?
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If -> When and if
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stripe just funded a from-scratch, beautifully documented resolver that is now shared between Bundler and CocoaPods. Why not use it in Rubygems as well?
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not indeed!
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c2aebd9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.