Support high water mark temporal flame graphs #326
Conversation
|
@pablogsal I'd appreciate it if you could skim through this sometime soon and share your initial impressions. I'm a bit worried that you'll be unhappy with how complex But hey, at least this one's only about 1000 lines! |
Aha 🤔 |
|
|
|
More than 400 LoC out of that are new tests Let's make some time to chat about what to do with this. I'd rather not release temporal leaks without also releasing temporal high water mark reporting... And given #382 we probably should cut a release soon. I'm tempted to back out the temporal leaks stuff (at least the CLI entry point for it and any docs) until we're comfortable landing this. I'm gonna rebase this, and also take another pass and see if I can find any ways to simplify it. |
godlygeek
force-pushed
the
temporal_hwm
branch
3 times, most recently
from
b332bea
to
b6e801b
Compare
|
@godlygeek You can rebase this now :) |
|
Done. I'm still tweaking this a bit, so I'm going to leave it as draft, but I'd love it if you could take another look at the first commit, in particular, and see if there's anything that stands out to you as wrong or bad or difficult to understand. I think I need to add more comments, and maybe some diagrams, but the implementation should remain basically unchanged |
src/memray/_memray/snapshot.cpp
Outdated
| size_t last_snapshot_peak = highest_peak_by_snapshot.at(history.last_known_snapshot); | ||
| history.rebase(last_snapshot_peak); | ||
|
|
||
| HistoricalContribution hc; |
There was a problem hiding this comment.
Why not using the brace-constructor here?
src/memray/_memray/snapshot.cpp
Outdated
| hc.contrib.allocations = history.allocations_contributed_to_last_known_peak; | ||
| hc.contrib.bytes = history.bytes_contributed_to_last_known_peak; | ||
|
|
||
| bool skip; |
There was a problem hiding this comment.
Let's rename this to skip_last_contribution or something similar so is clear what we are skipping
src/memray/_memray/snapshot.cpp
Outdated
| HistoricalContribution hc; | ||
| hc.as_of_snapshot = final.last_known_snapshot; | ||
| hc.peak_index = current_peak; | ||
| hc.contrib = hwm; | ||
| ret.push_back(hc); |
There was a problem hiding this comment.
We can create the element in-pace using emplace_back no?
src/memray/_memray/snapshot.cpp
Outdated
| Contribution hwm; | ||
| hwm.allocations = final.allocations_contributed_to_last_known_peak; | ||
| hwm.bytes = final.bytes_contributed_to_last_known_peak; |
src/memray/_memray/snapshot.cpp
Outdated
| HistoricalContribution hc; | ||
| hc.as_of_snapshot = final.last_known_snapshot + 1; | ||
| hc.peak_index = -1; | ||
| hc.contrib = leaks; | ||
| ret.push_back(hc); |
There was a problem hiding this comment.
Same as before: We can create the element in-pace using emplace_back
godlygeek
force-pushed
the
temporal_hwm
branch
2 times, most recently
from
b9a4ead
to
3de3321
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #326 +/- ##
==========================================
+ Coverage 84.28% 85.11% +0.82%
==========================================
Files 29 29
Lines 3520 3622 +102
==========================================
+ Hits 2967 3083 +116
+ Misses 553 539 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Update `HighWaterMarkAggregator` to be able to report on the high water mark within each snapshot, instead of only the overall high water mark. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Add a `TemporalHighWaterMarkAggregatorTestHarness` to the extension module, and use this to introduce a series of tests proving that we can correctly track the high water mark within each snapshot. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Currently Memray will never produce this type of report, but a future commit will modify the flamegraph reporter to begin writing HTML files that use this type of report. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Allow `memray flamegraph` to accept the `--temporal` argument without `--leaks` in order to generate a temporal high water mark report. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Building on top of the work to support our temporal flame graphs for analyzing memory leaked within a user-selected range, allow creating temporal flame graphs for analyzing the high water mark within a user-selected range.