Consider removing all allocations during profiler_get_backtrace
Categories
(Core :: Gecko Profiler, task, P3)
Tracking
()
People
(Reporter: gregtatum, Unassigned)
References
(Blocks 1 open bug)
Details
This will allow us to sample the stacks during allocations. This could be feasible to do in conjunction with the BlocksRingBuffer work.
Agreed!
Though I'd like to think about what we want to achieve at a higher level, because profiler_get_backtrace() returns UniqueProfilerBacktrace, which is a heap-based object, which itself points at more heap objects, so I don't think we can easily change all this (because there are callers that want to capture a trace at one point, and keep it for later).
I believe your goal is to capture a backtrace straight into a marker, right?
Currently this is done by calling profiler_get_backtrace(), and passing the UniqueProfilerBacktrace to a ProfilerMarkerPayload-derived object.
Instead we need to somehow reserve some stack space to collect the backtrace, and pass that by non-owning pointer or reference to a ProfilerMarkerPayload, so it's stored directly into the BlocksRingBuffer.
Most of the pieces are there (once bug 1562604 lands), we'll "just" need something like ProfilerBacktrace but with non-owning pointers/references instead of UniquePtrs, and then we can use a stack-based BlocksRingBuffer and ProfileBuffer.
To be further designed and prototyped...
|
Reporter | |
Comment 2•5 years ago
|
||
I believe your goal is to capture a backtrace straight into a marker, right?
Correct!
From bug 1645651: https://share.firefox.dev/3d2j7D7 is a profile that shows a lot of memory activity caused by profiler_get_backtrace() when "nativeallocations" is on. So it would be really nice to eliminate these allocations if possible.
I'm hoping that the new C++ API for Markers 2.0 can help, by handling the backtrace-capture in a smarter way. I'll add a note in bug 1646266.
Update:
Since bug 1646266, there should only be one allocation left, the one that contains a small-ish temporary ProfileChunkedBuffer where the trace is first stored before being copied into the global ProfileChunkedBuffer.
I'm hoping it should be possible to allocate it directly on the stack in most cases, with a fallback to the heap if really necessary (not enough stack space left, and/or too-small initial buffer.)
Since markers are getting more popular, and a few are taking stack traces, they become more noticeable in periodic samples, so I'm upping the priority of this task.
Some profiles by Bas showing this issue, from bug 1734867 comment 0:
https://share.firefox.dev/3FrP5rW
https://share.firefox.dev/3ljM19l
https://share.firefox.dev/3ljMkkv
Bug 1734867 removed an unexpected&unwanted allocation in temporary ProfileBuffers, and uses a pre-allocated buffer in the main thread of each process. This should take care of most cases, so I'm lowering the priority here to P3.
This bug can stay open, to potentially improve things further, e.g.:
- Detect how much stack space is left, and if sufficient, allocate the temporary buffer in it.
- Use other pre-allocated buffers for non-main threads (e.g., a first-come-first-serve pool, or explicitly-given buffers in some threads).
Description
•