1658232 - Add non-allocating profiler_capture_backtrace(ProfileChunkedBuffer&)

Status: RESOLVED FIXED

(Core :: Gecko Profiler, task, P2)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

As part of the new Markers C++ API, it will be possible for callers to request a backtrace.
But instead of making the callers do the capture and passing it through marker payloads, the new add-marker API will do the work as requested. Note that capturing a backtrace makes at least 4 allocations: The ProfilerBacktrace object itself, a ProfileBuffer, the underlying ProfileChunkedBuffer, and a single chunk.
Thanks to this delayed request, the add-marker code should be able to use the stack instead of most allocations. So we need to update ProfilerBacktrace so that it can live on the stack, and its dependencies as well.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1658232 - Disable native stack-walking in baseprofiler on linux - r?canaltinova,gregtatum

The upcoming changes will make it easier to try and capture backtraces from the baseprofiler. Unfortunately, the support is not there on Linux (including Android, and BSD), and trying to capture a backtrace would just crash in Registers::SyncPopulate() : https://searchfox.org/mozilla-central/rev/e07d5eb5646b40b10df3164e315d4a3108cf9101/mozglue/baseprofiler/core/platform-linux-android.cpp#542

So until this support is added, I will just prevent any stack-tracing there.

Depends on D86505

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1658232 - ns...Strings are now serialized with their length instead of the number of bytes - r?canaltinova,gregtatum

While working on this bug, I found it distracting to reason about how strings (of different char types) are stored in the profile buffer.
So instead of storing the size in bytes, I think it's better to store the string length in number of characters, in particular it matches lengths as handled by string types.

Depends on D86506

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1658232 - ProfileBufferEntryKinds.h - r?canaltinova,gregtatum

Backtraces and other marker data will be stored directly into a ProfileChunkedBuffer from public code in both profilers, so we will need to have the entry "kinds" available outside of the profiler directories.
This also helps with de-duplicating, since the kinds will now be in one spot and shared by both profilers.

Depends on D86507

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1658232 - Don't make ~ProfileBuffer() erase the ProfileChunkedBuffer - r?canaltinova,gregtatum

Until now the ProfileBuffer would erase its attached ProfileChunkedBuffer upon destruction.
However:

• The main ProfileChunkedBuffer is erased anyway in the ActivePS,
• Other ProfileChunkedBuffers are short-lived and don't really need to be erased.
• The upcoming changes to ProfilerBacktrace and its users means that we will only keep the ProfileChunkedBuffer as backtrace storage, a ProfileBuffer will only be needed during capture and then when streaming to JSON; so we don't want the ProfileChunkedBuffer to be erased when detached from its capturing ProfileBuffer.
• Also, the erasing was done by ResetChunkManager() in ~ProfileBuffer(), which was asymetric with what the constructor does (nothing!). So it's better to leave whoever did the SetChunkManager() to deal with the corresponding ResetChunkManager() (in the main case ActivePS, otherwise short-lived buffers being destroyed at the end of their scope).

Both ProfileBuffer destructors were only doing this operation, so we can just remove them completely.

Depends on D86508

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1658232 - ProfileBuffer::UnderlyingChunkedBuffer() - r?canaltinova,gregtatum

Let ProfilerBuffer expose its underlying ProfileChunkedBuffer, this will be useful when ProfilerBacktrace will only be given a ProfileBuffer, and to perform some safety checks.

As a bonus from this change, StoreMarker() can be made non-generic -- It was relying on both ProfileBuffer and ProfileChunkedBuffer to have the same function PutObjects(). Consequently, we don't need ProfileBuffer::PutObjects() anymore, this removes this clunky pass-through (but useful and the best solution at the time).

Depends on D86509

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1658232 - Use std::string for ProfilerBacktrace::mName - r?canaltinova,gregtatum

Instead of keeping a pointer to a null-terminated string, it's simpler to keep a proper std::string, and it helps to keep the length ready for streaming.

Depends on D86510

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1658232 - ProfilerBacktrace can reference or own a ProfileBuffer and/or ProfileChunkedBuffer - r?canaltinova,gregtatum

Instead of always taking ownership of both heap-allocated ProfileBuffer and ProfileChunkedBuffer, ProfilerBacktrace can now accept:

• Unique pointers to both or either, similar to what it was before, so a ProfilerBacktrace can be kept for later use.
• Non-owning references to both or either, to allow callers to use stack-based buffer(s).

Only the ProfileChunkedBuffer contains the actual data, we can create a ProfileBuffer on the spot if not provided.

Depends on D86511

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1658232 - Use a temporary ProfileBuffer in locked_profiler_get_backtrace - r?canaltinova,gregtatum

A heap-allocate ProfileBuffer is not really needed, so it's more efficient to have one on the stack during capture, and we don't need to keep it in the ProfilerBacktrace.

Depends on D86512

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1658232 - profiler_capture_backtrace(ProfileChunkedBuffer&) - r?canaltinova,gregtatum

The stack sampling can be abstracted to only use a reference to a ProfileBuffer, and the existing locked_profiler_get_backtrace can provide its stack-based ProfileBuffer that points at a heap-based ProfileChunkedBuffer (the one that will be stored in the returned ProfilerBacktrace).

And we can now add a public profiler_capture_backtrace that only takes a reference to a ProfileChunkedBuffer, and fills it with a backtrace.
This will be used by the new marker API, to optionally capture a backtrace in stack-based buffers at the user's request.

Depends on D86513

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64dc7599af5f
Disable native stack-walking in baseprofiler on linux - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/dd7c1416b90b
ns...Strings are now serialized with their length instead of the number of bytes - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/da7c87d165e7
ProfileBufferEntryKinds.h - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/887feeb16423
Don't make ~ProfileBuffer() erase the ProfileChunkedBuffer - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/f63ecf483b88
ProfileBuffer::UnderlyingChunkedBuffer() - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/edfba5362897
Use std::string for ProfilerBacktrace::mName  - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/07760d55d37b
ProfilerBacktrace can reference or own a ProfileBuffer and/or ProfileChunkedBuffer - r=canaltinova,gregtatum
https://hg.mozilla.org/integration/autoland/rev/4708c446cd96
Use a temporary ProfileBuffer in locked_profiler_get_backtrace - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/7aa58ee0e492
profiler_capture_backtrace(ProfileChunkedBuffer&) - r=canaltinova

Comment 11

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/64dc7599af5f
https://hg.mozilla.org/mozilla-central/rev/dd7c1416b90b
https://hg.mozilla.org/mozilla-central/rev/da7c87d165e7
https://hg.mozilla.org/mozilla-central/rev/887feeb16423
https://hg.mozilla.org/mozilla-central/rev/f63ecf483b88
https://hg.mozilla.org/mozilla-central/rev/edfba5362897
https://hg.mozilla.org/mozilla-central/rev/07760d55d37b
https://hg.mozilla.org/mozilla-central/rev/4708c446cd96
https://hg.mozilla.org/mozilla-central/rev/7aa58ee0e492