New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drop millions of allocations by using a linked list #1188

Merged
merged 1 commit into from Mar 13, 2015

Conversation

Projects
None yet
@tenderlove
Contributor

tenderlove commented Mar 13, 2015

I'm sending a PR because the reduction in allocations is so surprising to me that I'm afraid it's wrong. All tests pass on my machine, and I think it's backwards compatible, but I need review. /cc @evanphx @drbrain

Use a linked list to drop array allocations. The previous
implementation would dup arrays on every call to traverse. This patch
uses a linked list so that any block that is interested in keeping a
reference to trail can just keep the trail node around (the trail node
points at it's parents).

This approach reduces allocations from 11173668, to 2940.

Here is the test I used:

require 'stackprof'
require 'allocation_tracer'
require 'rubygems/test_case'
require 'rubygems/ext'
require 'rubygems/specification'
require 'benchmark'

class TestGemSpecification < Gem::TestCase
  def test_runtime
    make_gems do
      StackProf.run(mode: :wall, out: '/tmp/out.dump') do
        assert_raises(LoadError) { require 'no_such_file_foo' }
      end
    end
  end

  def test_alone
    make_gems do
      tms = Benchmark.measure {
        assert_raises(LoadError) { require 'no_such_file_foo' }
      }
      p tms.total
      assert_operator tms.total, :<=, 10
    end
  end

  def test_memory
    make_gems do
      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 array_alloc: ObjectSpace::AllocationTracer.allocated_count_table[:T_ARRAY]
      p :TOTAL => ObjectSpace::AllocationTracer.allocated_count_table.values.inject(:+)
    end
  end

  def make_gems
    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

      yield
    end
  end
end

Before:

{:TOTAL=>11173668}

After:

{:TOTAL=>2940}

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 13, 2015

Contributor

I think this will fix #1169 and #1167

Contributor

tenderlove commented Mar 13, 2015

I think this will fix #1169 and #1167

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 13, 2015

Contributor

Also the runtime test:

master:

[aaron@TC rubygems (master)]$ ruby -I lib:test test.rb -n test_runtime 
Run options: -n test_runtime --seed 52721

# Running tests:

.

Finished tests in 34.727601s, 0.0288 tests/s, 0.0288 assertions/s.

1 tests, 1 assertions, 0 failures, 0 errors, 0 skips

This branch:

[aaron@TC rubygems (omg)]$ ruby -I lib:test test.rb -n test_runtime 
Run options: -n test_runtime --seed 7522

# Running tests:

.

Finished tests in 0.050260s, 19.8965 tests/s, 19.8965 assertions/s.

1 tests, 1 assertions, 0 failures, 0 errors, 0 skips
Contributor

tenderlove commented Mar 13, 2015

Also the runtime test:

master:

[aaron@TC rubygems (master)]$ ruby -I lib:test test.rb -n test_runtime 
Run options: -n test_runtime --seed 52721

# Running tests:

.

Finished tests in 34.727601s, 0.0288 tests/s, 0.0288 assertions/s.

1 tests, 1 assertions, 0 failures, 0 errors, 0 skips

This branch:

[aaron@TC rubygems (omg)]$ ruby -I lib:test test.rb -n test_runtime 
Run options: -n test_runtime --seed 7522

# Running tests:

.

Finished tests in 0.050260s, 19.8965 tests/s, 19.8965 assertions/s.

1 tests, 1 assertions, 0 failures, 0 errors, 0 skips
@kuldeepaggarwal

This comment has been minimized.

Show comment
Hide comment
@kuldeepaggarwal

kuldeepaggarwal commented Mar 13, 2015

@tenderlove: Awesome.. 👍

@jcutrell

This comment has been minimized.

Show comment
Hide comment
@jcutrell

jcutrell Mar 13, 2015

This is one of the coolest merge requests I've seen for so many reasons.

jcutrell commented Mar 13, 2015

This is one of the coolest merge requests I've seen for so many reasons.

@railsme

This comment has been minimized.

Show comment
Hide comment
@railsme

railsme Mar 13, 2015

Yeah! Really awesome! 👍 👍 👍

railsme commented Mar 13, 2015

Yeah! Really awesome! 👍 👍 👍

@evanphx

This comment has been minimized.

Show comment
Hide comment
@evanphx

evanphx Mar 13, 2015

Member

I'd believe it. But look at the linked list that resolver uses for exactly the same reasons, maybe we can use it here too.

Member

evanphx commented Mar 13, 2015

I'd believe it. But look at the linked list that resolver uses for exactly the same reasons, maybe we can use it here too.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 13, 2015

Contributor

@evanphx where can I find it? I don't know the resolver code very well.

Contributor

tenderlove commented Mar 13, 2015

@evanphx where can I find it? I don't know the resolver code very well.

@tomciopp

This comment has been minimized.

Show comment
Hide comment

tomciopp commented Mar 13, 2015

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 13, 2015

Contributor

@tomciopp thanks!

I've updated the PR to use the built-in linked list class. I think we can implement the current instance method traverse in terms of the class method I've introduced (though ultimately I'd like to deprecate the instance method), but I will do that if this PR is acceptable for merging.

Contributor

tenderlove commented Mar 13, 2015

@tomciopp thanks!

I've updated the PR to use the built-in linked list class. I think we can implement the current instance method traverse in terms of the class method I've introduced (though ultimately I'd like to deprecate the instance method), but I will do that if this PR is acceptable for merging.

Show outdated Hide outdated lib/rubygems/specification.rb
stack = Gem::List.new(dep_spec, trail)
block[dep_spec, stack]
spec_name = dep_spec.name
traverse(dep_spec, stack, &block) unless

This comment has been minimized.

@skunkworker

skunkworker Mar 13, 2015

I may be wrong but shouldn't this be _traverse instead?

@skunkworker

skunkworker Mar 13, 2015

I may be wrong but shouldn't this be _traverse instead?

This comment has been minimized.

@libo

libo Mar 13, 2015

Adding a?

private_class_method :_traverse
@libo

libo Mar 13, 2015

Adding a?

private_class_method :_traverse
@libo

This comment has been minimized.

Show comment
Hide comment
@libo

libo commented Mar 13, 2015

👍

@chus

This comment has been minimized.

Show comment
Hide comment
@chus

chus commented Mar 13, 2015

👍

@@ -2,6 +2,8 @@ module Gem
List = Struct.new(:value, :tail)
class List
include Enumerable

This comment has been minimized.

@byroot

byroot Mar 13, 2015

If you include Enumerable you don't need the explicit find implementation anymore. The to_a implementation could go too if Struct didn't have a default one.

@byroot

byroot Mar 13, 2015

If you include Enumerable you don't need the explicit find implementation anymore. The to_a implementation could go too if Struct didn't have a default one.

This comment has been minimized.

@tenderlove

tenderlove Mar 13, 2015

Contributor

I'd rather do that in a different commit. 😨

@tenderlove

tenderlove Mar 13, 2015

Contributor

I'd rather do that in a different commit. 😨

This comment has been minimized.

@ms-ati

ms-ati Mar 19, 2015

Contributor

@byroot This isn't quote true. You have to remove Struct (which has its own Enumerable inclusion and meaning), and reverse the to_a. See PR #1200 ;)

@ms-ati

ms-ati Mar 19, 2015

Contributor

@byroot This isn't quote true. You have to remove Struct (which has its own Enumerable inclusion and meaning), and reverse the to_a. See PR #1200 ;)

@pawitk

This comment has been minimized.

Show comment
Hide comment
@pawitk

pawitk commented Mar 13, 2015

👍

@tenderlove tenderlove closed this Mar 13, 2015

@tenderlove tenderlove reopened this Mar 13, 2015

@libo

This comment has been minimized.

Show comment
Hide comment
@libo

libo Mar 13, 2015

Is the merge button you have to click @tenderlove,

libo commented Mar 13, 2015

Is the merge button you have to click @tenderlove,

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 13, 2015

Contributor

@libo hah, I closed and reopened to get travis to build again.

Contributor

tenderlove commented Mar 13, 2015

@libo hah, I closed and reopened to get travis to build again.

@libo

This comment has been minimized.

Show comment
Hide comment
@libo

libo Mar 13, 2015

:-) eheh

By this PR was a hell of a #fridayhug thanks @tenderlove !

libo commented Mar 13, 2015

:-) eheh

By this PR was a hell of a #fridayhug thanks @tenderlove !

@canweriotnow

This comment has been minimized.

Show comment
Hide comment

canweriotnow commented Mar 13, 2015

tumblr_inline_mzmuzndcac1qi4qb3

@draffauf

This comment has been minimized.

Show comment
Hide comment
@draffauf

draffauf Mar 13, 2015

Mind blown. Great job.

draffauf commented Mar 13, 2015

Mind blown. Great job.

drop millions of allocations by using a linked list
Use a linked list to drop array allocations.  The previous
implementation would dup arrays on every call to `traverse`.  This patch
uses a linked list so that any block that is interested in keeping a
reference to `trail` can just keep the trail node around (the trail node
points at it's parents).

This approach reduces allocations from 11173668, to 2940.

Here is the test I used:

```ruby
require 'stackprof'
require 'allocation_tracer'
require 'rubygems/test_case'
require 'rubygems/ext'
require 'rubygems/specification'
require 'benchmark'

class TestGemSpecification < Gem::TestCase
  def test_runtime
    make_gems do
      StackProf.run(mode: :wall, out: '/tmp/out.dump') do
        assert_raises(LoadError) { require 'no_such_file_foo' }
      end
    end
  end

  def test_alone
    make_gems do
      tms = Benchmark.measure {
        assert_raises(LoadError) { require 'no_such_file_foo' }
      }
      p tms.total
      assert_operator tms.total, :<=, 10
    end
  end

  def test_memory
    make_gems do
      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 array_alloc: ObjectSpace::AllocationTracer.allocated_count_table[:T_ARRAY]
      p :TOTAL => ObjectSpace::AllocationTracer.allocated_count_table.values.inject(:+)
    end
  end

  def make_gems
    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

      yield
    end
  end
end
```

Before:

{:TOTAL=>11173668}

After:

{:TOTAL=>2940}
@sstelfox

This comment has been minimized.

Show comment
Hide comment
@sstelfox

sstelfox Mar 13, 2015

Wow awesome. Nice find!

sstelfox commented Mar 13, 2015

Wow awesome. Nice find!

@renatomoya

This comment has been minimized.

Show comment
Hide comment
@renatomoya

renatomoya commented Mar 13, 2015

Way to go @tenderlove

@vcavallo

This comment has been minimized.

Show comment
Hide comment
@vcavallo

vcavallo commented Mar 13, 2015

8 O

@amaltson

This comment has been minimized.

Show comment
Hide comment
@amaltson

amaltson Mar 13, 2015

Awesome work @tenderlove, can't wait for this to be released 😄

amaltson commented Mar 13, 2015

Awesome work @tenderlove, can't wait for this to be released 😄

@gbxl

This comment has been minimized.

Show comment
Hide comment
@gbxl

gbxl Mar 13, 2015

Dude that's awesome! Good job.

gbxl commented Mar 13, 2015

Dude that's awesome! Good job.

@zeeraw

This comment has been minimized.

Show comment
Hide comment
@zeeraw

zeeraw Mar 13, 2015

@tenderlove No matter what people on the internet say, what you're doing is fantastic. Please don't get discouraged. Thank you for spotting and optimising this! #FridayHug

zeeraw commented Mar 13, 2015

@tenderlove No matter what people on the internet say, what you're doing is fantastic. Please don't get discouraged. Thank you for spotting and optimising this! #FridayHug

stack = Gem::List.new(dep_spec, trail)
block[dep_spec, stack]
spec_name = dep_spec.name
_traverse(dep_spec, stack, &block) unless

This comment has been minimized.

@seandmccarthy

seandmccarthy Mar 13, 2015

I may have missed something, but I think this should be traverse rather than _traverse as the former is the only one (I can see) with a 3 element signature.

@seandmccarthy

seandmccarthy Mar 13, 2015

I may have missed something, but I think this should be traverse rather than _traverse as the former is the only one (I can see) with a 3 element signature.

This comment has been minimized.

@cheald

cheald Mar 13, 2015

I think you have that backwards; traverse takes 1 param + block, _traverse takes 2 + block. And in any case the Gem::List is generated prior to the invocation, so traverse wouldn't be appropriate anyhow.

@cheald

cheald Mar 13, 2015

I think you have that backwards; traverse takes 1 param + block, _traverse takes 2 + block. And in any case the Gem::List is generated prior to the invocation, so traverse wouldn't be appropriate anyhow.

This comment has been minimized.

@seandmccarthy

seandmccarthy Mar 13, 2015

Yeah sorry, scratch that. I was looking at an earlier version and the call is correct now.

@seandmccarthy

seandmccarthy Mar 13, 2015

Yeah sorry, scratch that. I was looking at an earlier version and the call is correct now.

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald commented Mar 13, 2015

👍

tenderlove added a commit that referenced this pull request Mar 13, 2015

Merge pull request #1188 from tenderlove/omg
drop millions of allocations by using a linked list

@tenderlove tenderlove merged commit 7d314e3 into rubygems:master Mar 13, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mfazekas

This comment has been minimized.

Show comment
Hide comment
@mfazekas

mfazekas Mar 13, 2015

Contributor

Hm.. i'm pretty sure some cheating is going on here. I don't see the trick, but you decreased the number of allocations because you fixed the exponential algorithm.

I see it:

stack = Gem::List.new(dep_spec, trail)
spec_name = dep_spec.name
unless stack.any? { |s| s.name == spec_name }

stack contains spec_name so we'll never recurse. You need to check trail.any?

so you need to change

unless stack.any? { |s| s.name == spec_name }

to

unless trail.any? { |s| s.name == spec_name }
Contributor

mfazekas commented on b19051c Mar 13, 2015

Hm.. i'm pretty sure some cheating is going on here. I don't see the trick, but you decreased the number of allocations because you fixed the exponential algorithm.

I see it:

stack = Gem::List.new(dep_spec, trail)
spec_name = dep_spec.name
unless stack.any? { |s| s.name == spec_name }

stack contains spec_name so we'll never recurse. You need to check trail.any?

so you need to change

unless stack.any? { |s| s.name == spec_name }

to

unless trail.any? { |s| s.name == spec_name }
@dainmiller

This comment has been minimized.

Show comment
Hide comment
@dainmiller

dainmiller commented Mar 13, 2015

👍

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 13, 2015

Contributor

@mfazekas you are totally right. Think we should revert?

Contributor

tenderlove commented Mar 13, 2015

@mfazekas you are totally right. Think we should revert?

@alloy alloy referenced this pull request Mar 15, 2015

Merged

Support what RubyGems needs #20

@mfazekas

This comment has been minimized.

Show comment
Hide comment
@mfazekas

mfazekas Mar 16, 2015

Contributor

@tenderlove i think it have to be reverted, and we need to add some test that fails with that implementation.

Contributor

mfazekas commented Mar 16, 2015

@tenderlove i think it have to be reverted, and we need to add some test that fails with that implementation.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 16, 2015

Contributor

@mfazekas do you have any idea how to make a test that fails with this implementation? I don't want to revert until we can prove it's wrong (no doubt it is wrong, just need to make a test).

Contributor

tenderlove commented Mar 16, 2015

@mfazekas do you have any idea how to make a test that fails with this implementation? I don't want to revert until we can prove it's wrong (no doubt it is wrong, just need to make a test).

@mfazekas

This comment has been minimized.

Show comment
Hide comment
@mfazekas

mfazekas Mar 16, 2015

Contributor

i'll try to came up with a testcase sometime later today

Contributor

mfazekas commented Mar 16, 2015

i'll try to came up with a testcase sometime later today

mfazekas added a commit to mfazekas/rubygems that referenced this pull request Mar 17, 2015

@mfazekas

This comment has been minimized.

Show comment
Hide comment
@mfazekas

mfazekas Mar 17, 2015

Contributor

#1191 shows is a testcase, demonstrating the issue with your optimization. But it also shows that the original one despite all the multiple reverse's and exponential algorithm will prefer earlier and not later versions.

So we use find_in_unresolved_tree in case we cannot find a required file in any of the activated gems. The idea is that we check indirect dependencies of the activated gems and try to select a gem that contains the file, but doesn't cause a conflict. And from all of the possibilities we should prefer latest gem versions.

Current implementation is has many issues as its exponential, might select conflicting gems (#1169) and might not use the latest versions of the possibilities (#1191).

Contributor

mfazekas commented Mar 17, 2015

#1191 shows is a testcase, demonstrating the issue with your optimization. But it also shows that the original one despite all the multiple reverse's and exponential algorithm will prefer earlier and not later versions.

So we use find_in_unresolved_tree in case we cannot find a required file in any of the activated gems. The idea is that we check indirect dependencies of the activated gems and try to select a gem that contains the file, but doesn't cause a conflict. And from all of the possibilities we should prefer latest gem versions.

Current implementation is has many issues as its exponential, might select conflicting gems (#1169) and might not use the latest versions of the possibilities (#1191).

@twelve17

This comment has been minimized.

Show comment
Hide comment
@twelve17

twelve17 Mar 19, 2015

Some 👏 for @mfazekas too!

twelve17 commented Mar 19, 2015

Some 👏 for @mfazekas too!

@sachinprasad

This comment has been minimized.

Show comment
Hide comment
@sachinprasad

sachinprasad Mar 19, 2015

A valid reason to learn linked lists 👍

sachinprasad commented Mar 19, 2015

A valid reason to learn linked lists 👍

ms-ati pushed a commit to ms-ati/rubygems that referenced this pull request Mar 19, 2015

DRY up util Gem::List
1. Methods `#find` and `#to_a` can use the implementations
defined in Enumerable. 

2. The inclusion of Enumerable in Struct was the obstacle,
so remove the Struct and define a constructor instead, in
roughly the same number of lines of code.

3. Avoid potential N^2 behavior in previous `to_a` implementation.
Why? `Array#unshift` has linear performance, so iteratively
building a reversed array by unshifting risks N^2 performance.
Instead, do a single `#reverse` for linear at worst.

(I head about this code from #1188 via Ruby Weekly news)

@ms-ati ms-ati referenced this pull request Mar 19, 2015

Merged

DRY up Gem::List in utils #1200

@ms-ati

This comment has been minimized.

Show comment
Hide comment
@ms-ati

ms-ati Mar 19, 2015

Contributor

Hi @tenderlove, very cool.

After seeing this commit on Ruby Weekly News, I took a look at this code for the first time. I just submitted a PR to hopefully improve the linked list implementation used here (#1200).

Specifically, in the #to_a code path that is frequently called, Array#unshift was called for every list element. I believe this is quadratic performance, since unshift is linear. Is that right?

Contributor

ms-ati commented Mar 19, 2015

Hi @tenderlove, very cool.

After seeing this commit on Ruby Weekly News, I took a look at this code for the first time. I just submitted a PR to hopefully improve the linked list implementation used here (#1200).

Specifically, in the #to_a code path that is frequently called, Array#unshift was called for every list element. I believe this is quadratic performance, since unshift is linear. Is that right?

ms-ati added a commit to ms-ati/rubygems that referenced this pull request Apr 15, 2015

DRY up util Gem::List
1. Methods `#find` and `#to_a` can use the implementations
defined in Enumerable. 

2. The inclusion of Enumerable in Struct was the obstacle,
so remove the Struct and define a constructor instead, in
roughly the same number of lines of code.

3. Avoid potential N^2 behavior in previous `to_a` implementation.
Why? `Array#unshift` has linear performance, so iteratively
building a reversed array by unshifting risks N^2 performance.
Instead, do a single `#reverse` for linear at worst.

(I head about this code from #1188 via Ruby Weekly news)
@007lva

This comment has been minimized.

Show comment
Hide comment
@007lva

007lva commented Apr 18, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment