1734867 - Considerable overhead from allocating temporary profiler buffer when capturing markers with stack

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P2)

Description

[Bas Schouten (:bas.schouten)]

We're seeing capturing markers with stack cause significant overhead due to memory allocation.

Example profiles:

https://share.firefox.dev/3FrP5rW

https://share.firefox.dev/3ljM19l

https://share.firefox.dev/3ljMkkv

Comment 1

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

I believe this is due to the last allocation (that I know of) in markers, see some details in bug 1578792 comment 4. I'll copy your profiles there.

We use a ProfilerBufferChunkManagerSingle in AddMarkerToBuffer, and that allocates a chunk on the heap.

Ideally, we'd like to allocate this buffer on the stack, if there is enough space left -- that last part may be difficult to implement on all platform(?)

An alternative would be to have one or more pre-allocated buffers that could be used.
The best would be one per profiled thread, but that could chew quite a bit of memory if profiling lots of threads.
Otherwise a pool of available buffers, at some small synchronization cost, with the worst case being that they're all being used and block more markers.

Comment 2

[Florian Quèze [:florian]]

(In reply to Gerald Squelart [:gerald] (he/him) from comment #1)

The best would be one per profiled thread, but that could chew quite a bit of memory if profiling lots of threads.

For the example profiles provided in comment 0, it seems only the main thread matters, so special casing it might make sense.

Comment 3

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

On it now.

I will add a main-thread path in AddMarkerToBuffer using a single static buffer that's only allocated once, when needed by the very first main-thread marker.

And while working on this, I discovered/remembered that all ProfileBuffers allocated a chunk that was only really needed in some situations, so we can avoid this allocation in temporary ProfileBuffers like in AddMarkerToBuffer; this will benefit all threads. 🎉

Comment 4

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

Attachment: Bug 1734867 - Allocate ProfilerBuffer's worker ChunkManager only when first needed - r?florian

Some ProfileBuffers are temporary and don't actually need this allocation.

Comment 5

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

Attachment: Bug 1734867 - Refactor stack-capturing path in AddMarkerToBuffer - r?florian

The resulting code should be the same. The factored lambda will be called in the next patch with a different chunked buffer.

Depends on D133723

Comment 6

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

Attachment: Bug 1734867 - Use single static buffer when capturing stacks for main-thread markers - r?florian

The main thread is the busiest, so it benefits the most from having its own chunked buffer. This removes all but one allocation when capturing a marker stack on the main thread of each process.

Note: Further improvements are possible (e.g.: Pool of pre-allocated buffers, attempting to use a stack-based buffer, etc.), but they are more complex and will require more work in future bugs.

Depends on D133724

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46bfc82686ac
Allocate ProfilerBuffer's worker ChunkManager only when first needed - r=florian
https://hg.mozilla.org/integration/autoland/rev/17219f7b60f5
Refactor stack-capturing path in AddMarkerToBuffer - r=florian
https://hg.mozilla.org/integration/autoland/rev/63b2fd522aa8
Use single static buffer when capturing stacks for main-thread markers - r=florian

Comment 8

[Cristian Tuns]

Backed out for causing xpcshell failures on ProfileBufferChunkManager.h

• Backout link
• Push with failures
• Failure Log
• Failure line: Assertion failure: !mUser, at /builds/worker/workspace/obj-build/dist/include/mozilla/ProfileBufferChunkManager.h:117

Comment 9

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

Thank you Cristian. Strange that I didn't catch that one in my Try. I'll investigate...

Comment 10

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

Oh, it's because I didn't run xpcshell tests! 😅

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b3b1ab8d743
Allocate ProfilerBuffer's worker ChunkManager only when first needed - r=florian
https://hg.mozilla.org/integration/autoland/rev/7e01973ce58a
Refactor stack-capturing path in AddMarkerToBuffer - r=florian
https://hg.mozilla.org/integration/autoland/rev/bcddb167725c
Use single static buffer when capturing stacks for main-thread markers - r=florian

Comment 12

[Atila Butkovits]

Backed out for causing assertion failures at ProfilerMarkers.cpp.

Backout link: https://hg.mozilla.org/integration/autoland/rev/e390c6bc407cfc2c4cd8a78ec50719487c1842fb

Push where failures started: https://hg.mozilla.org/integration/autoland/rev/e390c6bc407cfc2c4cd8a78ec50719487c1842fb

Failure log: https://treeherder.mozilla.org/logviewer?job_id=361496637&repo=autoland&lineNumber=47160

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce69ff8b2ab1
Allocate ProfilerBuffer's worker ChunkManager only when first needed - r=florian
https://hg.mozilla.org/integration/autoland/rev/ec0aef0c5580
Refactor stack-capturing path in AddMarkerToBuffer - r=florian
https://hg.mozilla.org/integration/autoland/rev/5bd128b532ea
Use single static buffer when capturing stacks for main-thread markers - r=florian

Comment 14

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/ce69ff8b2ab1
https://hg.mozilla.org/mozilla-central/rev/ec0aef0c5580
https://hg.mozilla.org/mozilla-central/rev/5bd128b532ea