chore(profiling): convert memory profiler to C++ #15236
Conversation
danielsn
added
the
changelog/no-changelog
|
|
| void | ||
| memalloc_heap_tracker_init(uint32_t sample_size) | ||
| { | ||
| // TODO(dsn): what should we do it this was already initialized? |
There was a problem hiding this comment.
Do we take any risks by reusing the existing heap tracker?
| global_heap_tracker->deinit(); | ||
| delete global_heap_tracker; |
There was a problem hiding this comment.
Shouldn't deinit be (or be part of) the destructor?
There was a problem hiding this comment.
I feel like deinit is a bad name. The issue is that currently, deallocating the global_heap_tracker requires calls into cpython, which could release the GIL. So we need a way to stop recording those operations as we're deallocating. I'm not sure this is the right way tho. I will give that some thought.
|
|
||
| // memalloc_heap_map implementation | ||
| memalloc_heap_map::memalloc_heap_map() | ||
| : impl(new Impl()) |
There was a problem hiding this comment.
Would it make sense to have a unique_ptr<Impl> instead?
| }; | ||
|
|
||
| memalloc_heap_map::iterator::iterator() | ||
| : impl(new Impl()) |
There was a problem hiding this comment.
Same here with unique_ptr vs. raw pointer
| } heap_tracker_t; | ||
| }; | ||
|
|
||
| static heap_tracker_t global_heap_tracker; |
There was a problem hiding this comment.
To confirm, have you made sure we have everything needed to handle forks and correctly allocating a new global_heap_tracker on fork?
There was a problem hiding this comment.
Would it make sense to add unit tests for that container?
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 235 ± 3 ms. The average import time from base is: 241 ± 2 ms. The import time difference between this PR and base is: -5.3 ± 0.1 ms. Import time breakdownThe following import paths have shrunk:
|
Description
Converts the memory profiler from C to C++.
This enables us to use things like RAII to manage memory, and collections like std::vector instead of hand-rolled array lists.
There should be no functional differences.
Testing
Refactor change that is covered by existing tests.
Risks
Additional Notes