Skip to content
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

Should GC.start be run before reporting results? #12

Open
dblock opened this issue Sep 18, 2020 · 7 comments
Open

Should GC.start be run before reporting results? #12

dblock opened this issue Sep 18, 2020 · 7 comments

Comments

@dblock
Copy link
Contributor

dblock commented Sep 18, 2020

Coming from #9. Output was confusing without GC.start.

@michaelherold
Copy link
Owner

Good question, @dblock. I think that makes sense to me. Apologies for the delay in response!

@AlexWayfer
Copy link
Contributor

I can try to make a PR, if you want.

@dblock
Copy link
Contributor Author

dblock commented Nov 18, 2020

Don't let me stop by @AlexWayfer :)

@AlexWayfer
Copy link
Contributor

I'm interesting in fresh dependencies, so I want to wait for #11 and #10.

@AlexWayfer
Copy link
Contributor

Now #14 (and merged #16 with #17 would be good too).

@michaelherold
Copy link
Owner

Alright, digging in here. If I am understanding the intent of the benchmark, this is a clearer way to write it:

Benchmark code
require 'set'
require 'benchmark/memory'

class Retainer
  def initialize(container)
    @container = container
  end

  def retain!
    @container << { x: 1 }
  end
end

ra = Retainer.new([])
rs = Retainer.new(Set.new)

Benchmark.memory do |b|
  calls = 10_001

  b.report('using Array') do
    calls.times { ra.retain! }
  end

  b.report('using Set') do
    calls.times { rs.retain! }
  end

  b.compare!
end

Running this gives the following output:

Benchmark output
Calculating -------------------------------------
         using Array     2.320M memsize (     2.320M retained)
                        10.001k objects (    10.001k retained)
                         0.000  strings (     0.000  retained)
           using Set     2.321M memsize (   928.000  retained)
                        10.004k objects (     4.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
         using Array:    2320232 allocated
           using Set:    2320928 allocated - 1.00x more

If I'm understanding the source of the question, you were finding this confusing because the report is about the magnitude of allocations rather than the magnitude of retentions. In that case, I don't think that calling GC.start would do what you expect because it's already done as part of the measurement.

I think what I would like to do to address this is twofold:

  1. Output a comparison of retentions as well as allocations
  2. Allow you to use allocations OR retentions to sort the comparison

(1) would make the output less confusing by default and (2) would allow you to tune the tool to be better suited for testing memory leaks.

What do you think? Would those two changes have made the situation less confusing?

@AlexWayfer
Copy link
Contributor

I'm reviewing the original report and a bit confused:

Yeah, @setup << { x: 1 } with @setup as a Set will prevent duplicates inside itself, but there are { x: 1 } initializations anyway, and they're all in a report, so… objects created anyway. And now, while I'm writing it, I see 1.680M retained vs 168.000 retained, and yes, having their comparison under Comparison: "header" can make things more understandable. Also maybe renaming or a hint like "retained after garbage collection".

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

No branches or pull requests

3 participants