Implement Report#merge for efficiently combining many reports #110
Conversation
There was a problem hiding this comment.
This looks neat, and probably will help speed things up in the cases you are mentioning (most likely saving a hefty amount of excess object allocations).
I have one idea that you might find useful, but probably is in the land of "over optimization", and definitely a bit "ugly" from a code readability standpoint. Just something to consider.
| end | ||
|
|
||
| d1, d2 = data, other.data | ||
| data = { | ||
| all_data = all.map(&:data) |
There was a problem hiding this comment.
I feel like you could save a few iterations of this data by just calculating the samples, gc_samples, and missed_samples in a single loop:
# untested code... you have been warned!
new_samples, new_gc_samples, new_missed_samples = all.inject([0,0,0]) do |result, data|
result[0] += data[:samples]
result[1] += data[:gc_samples]
result[2] += data[:missed_samples]
result
end
new_data = {
# ...
samples: new_samples,
gc_samples: new_gc_samples,
missed_samples: new_missed_samples
}However, I realize it looks a bit uglier this way...
There was a problem hiding this comment.
I'm not sure it's worth optimizing this much since it's O(number of reports) whereas the main loop is O(frames) which should usually be many orders of magnitude larger.
There was a problem hiding this comment.
Yeah, also that. Just something I noticed when looking through the code that stuck out to me, but definitely agree that it probably isn't the big focal point of the optimization.
I think it can be adjusted at a second pass if it turns out to be beneficial, but I think what you have is more readable and fine to keep as is.
| report.normalized_frames.each do |id, frame| | ||
| if !merged[id] | ||
| merged[id] = frame | ||
| next |
There was a problem hiding this comment.
I like the early eject here. 👍
|
@NickLaMuro thanks for the review! |
|
As far as I can tell the test failures on recent Ruby versions are unrelated to this change. |
I need to combine 40 large profiles collected from a CI run.
Report#+gets very slow as I merge more reports (e.g. withreports.inject(:+)). This PR implementsReport#mergethat allows for merging an arbitrary number of reports in a single pass. It also implements a test for the merge behavior.