michaelherold / benchmark-memory

Memory profiling benchmark style, for Ruby 2.1+
MIT License
217 stars 6 forks source link

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

Open dblock opened 4 years ago

dblock commented 4 years ago

Coming from https://github.com/michaelherold/benchmark-memory/issues/9. Output was confusing without GC.start.

michaelherold commented 3 years ago

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

AlexWayfer commented 3 years ago

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

dblock commented 3 years ago

Don't let me stop by @AlexWayfer :)

AlexWayfer commented 3 years ago

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

AlexWayfer commented 3 years ago

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

michaelherold commented 3 years ago

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

Benchmark code ```ruby 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 commented 3 years ago

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".