Closed godlygeek closed 1 year ago
@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! 🥳
Anyway, there's tests now! 🥳
Ah, but someone forgot to signup the commits :P 😆
Ah, but someone forgot to signup the commits :P 😆
Ah, only the fixup, which I deliberately avoided signing to remind us that we needed to squash it :P
One thing I noticed, we should change the title in the flamegraph because right now it shows temporal_flamegraph
:
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.
Whoops, one more force push - I missed updating TemporalAllocationRecord
to not be a subclass of AllocationRecord
in the extension module's type stubs.
Whoops, one more force push - I missed updating
TemporalAllocationRecord
to not be a subclass ofAllocationRecord
in the extension module's type stubs.
Which also surfaced another type warning. I just shut it up with a # type: ignore
, since that's code that I plan on refactoring as soon as we land this PR.
And then, just for fun, I rebased on main
. 😂
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.