Temporal flamegraphs #317
Merged
Temporal flamegraphs #317
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
godlygeek
force-pushed
the
temporal_flamegraphs
branch
from
9422a29 to
d482f10
Compare
godlygeek
force-pushed
the
temporal_flamegraphs
branch
3 times, most recently
from
fb7695c to
e733d5f
Compare
godlygeek
force-pushed
the
temporal_flamegraphs
branch
2 times, most recently
from
8093b66 to
f4dbebe
Compare
|
@pablogsal Since you've started looking at this, I pushed one commit as a fixup that corrects some minor issues noticed while testing, rather than force pushing fixes to code you've already started reviewing. We'll need to make sure to apply the fixup before landing this. Anyway, there's tests now! 🥳 |
Ah, but someone forgot to signup the commits :P 😆 |
pablogsal
reviewed
Feb 17, 2023
pablogsal
reviewed
Feb 17, 2023
pablogsal
reviewed
Feb 17, 2023
pablogsal
reviewed
Feb 17, 2023
Ah, only the fixup, which I deliberately avoided signing to remind us that we needed to squash it :P |
pablogsal
reviewed
Feb 21, 2023
pablogsal
reviewed
Feb 21, 2023
pablogsal
reviewed
Feb 21, 2023
godlygeek
force-pushed
the
temporal_flamegraphs
branch
from
b1d5e5d to
0128bc2
Compare
pablogsal
reviewed
Feb 23, 2023
|
One thing I noticed, we should change the title in the flamegraph because right now it shows
|
pablogsal
reviewed
Feb 23, 2023
godlygeek
force-pushed
the
temporal_flamegraphs
branch
from
ba32ab1 to
70ee7cd
Compare
|
The force push I just did should address the new concerns you raised, plus some minor things I noticed: the unnecessary vector copy, our help button text not mentioning the sliders, our title not including "(memory leaks)", and the warning about generating the report without tracking Python allocators not showing up. |
godlygeek
force-pushed
the
temporal_flamegraphs
branch
from
70ee7cd to
3dadf7e
Compare
|
Whoops, one more force push - I missed updating |
godlygeek
force-pushed
the
temporal_flamegraphs
branch
from
3dadf7e to
ee2a1fb
Compare
Which also surfaced another type warning. I just shut it up with a |
For each snapshot, report the allocations performed between that snapshot and the next, grouped by location, and also report which snapshot each of those allocations is freed in. Signed-off-by: Matt Wozniski <[email protected]>
Create a template page for a new type of flame graph. Initially this is just an exactly copy of our existing flame graph template, to make the changes from the original stand out as diffs in future commits. Signed-off-by: Matt Wozniski <[email protected]>
Allow generating flame graphs with a temporal dimension, which are able to show the allocations performed in some range and not deallocated before the end of that range. Signed-off-by: Matt Wozniski <[email protected]>
`SIZE_MAX` is a helpful marker for us to work within the C++ layer, but translating this to `None` in the Python layer and `null` in the JavaScript layer makes things easier and more intuitive to work with, and avoids having a magic number (different for 32 vs 64 bit builds!) leak into our public interface. Signed-off-by: Matt Wozniski <[email protected]>
Signed-off-by: Matt Wozniski <[email protected]>
Previously we were postprocessing the aggregated records to handle the `merge_threads` option, but we can instead just preprocess the records fed into the aggregator, so that the aggregator uses the right key from the start. Signed-off-by: Matt Wozniski <[email protected]>
This will allow the same stack trace creation logic to be shared by multiple types of allocation records. Signed-off-by: Matt Wozniski <[email protected]>
Instead, make it its own type, analogous to `AllocationRecord` but without the `address`, `n_allocations`, and `size` properties, and instead with an `intervals` property that holding a list of intervals, each with an `n_allocations` and `n_bytes`, as well as an `allocated_before_snapshot` and `deallocated_before_snapshot` indicating which time ranges that allocation was alive for. Signed-off-by: Matt Wozniski <[email protected]>
Make some methods that always should have been `const` actually be `const`, and improve the comments to clarify why this method needs an intermediate hash table. Signed-off-by: Matt Wozniski <[email protected]>
When the report is first rendered, before the user has interacted with it at all, we want to show the same set of leaked allocations as the old, non-temporal leaks report would show. This means that we need to include deallocations that occurred after the final snapshot, but before tracking stopped. To achieve that, we need to adjust our treatment of the "end" index chosen by the user, including allocations and deallocations that occurred between that snapshot and the next one (in other words, treating it as inclusive rather than exclusive). Signed-off-by: Matt Wozniski <[email protected]>
Previously the rendered HTML contained "temporal_flamegraph report". Fix it to say "temporal flamegraph report (memory leaks)" instead, and to give our normal warning if the leaks report was generated without tracking Python allocators. Signed-off-by: Matt Wozniski <[email protected]>
Temporal flame graphs have just been inheriting the help text of normal flame graphs. Update this to add a small section explaining how to use the sliders to investigate a different time range. Signed-off-by: Matt Wozniski <[email protected]>
godlygeek
force-pushed
the
temporal_flamegraphs
branch
from
ee2a1fb to
5920389
Compare
|
And then, just for fun, I rebased on |
pablogsal
approved these changes
Feb 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm leaving off the news and docs for now, as I'd still like to do some more refactoring here (possibly putting this under a new subcommand, rather than putting it under a flag for the existing flamegraph reporter).
I should probably add some automated tests as well... but other than that, this should be in a reviewable state.