bloomberg / memray

Memray is a memory profiler for Python
https://bloomberg.github.io/memray/
Apache License 2.0
13.17k stars 392 forks source link

Fix temporal flame graph handling of HWM before the first or after the last snapshot #399

Closed pablogsal closed 1 year ago

pablogsal commented 1 year ago

closes: #396

pablogsal commented 1 year ago

I wonder if we should emit a warning or something here...

godlygeek commented 1 year ago

I wonder if we should emit a warning or something here...

Does this wind up showing an empty flame graph, or does it show a static flame graph that's equivalent to the non-temporal flame graph? If it's empty, we should fail at generation time, if it's static and the user just can't modify the time range because we only have one snapshot's worth of data, that's probably fine...

Your second commit, "Update package-lock.json and binary files", includes functional changes, not just auto-generated ones - it looks like you might have applied a fixup to the wrong commit?

pablogsal commented 1 year ago

Your second commit, "Update package-lock.json and binary files", includes functional changes, not just auto-generated ones - it looks like you might have applied a fixup to the wrong commit?

Fixed

Does this wind up showing an empty flame graph, or does it show a static flame graph that's equivalent to the non-temporal flame graph?

Is not empty, just the plot is empty

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -0.25 :warning:

Comparison is base (ae1fdde) 85.00% compared to head (8a95cb0) 84.75%.

:exclamation: Current head 8a95cb0 differs from pull request most recent head ef3b9bb. Consider uploading reports for the commit ef3b9bb to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #399 +/- ## ========================================== - Coverage 85.00% 84.75% -0.25% ========================================== Files 29 29 Lines 3621 3621 ========================================== - Hits 3078 3069 -9 - Misses 543 552 +9 ``` | Flag | Coverage Δ | | |---|---|---| | cpp | `84.75% <ø> (-0.25%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#carryforward-flags-in-the-pull-request-comment) to find out more. [see 5 files with indirect coverage changes](https://app.codecov.io/gh/bloomberg/memray/pull/399/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

godlygeek commented 1 year ago

Oh boy, there was a lot here that wasn't working great. We had problems when the high water mark was before the first snapshot, when it was after the last snapshot, when there were exactly 0 snapshots, and when there was exactly 1 snapshot.

I think this PR now fixes everything - at least, it fixes all the problems I could find. Take a look when you can, @pablogsal - there was enough wrong here that we should probably cut a bug fix release once we land this.

pablogsal commented 1 year ago

I reviewed your commits but I cannot accept my own PR 😅