LUL: increase size of unwound stack to 160k

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jseward, Assigned: jseward)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

For reasons related to the architecture of the Gecko Profiler in previous years,
which are no longer relevant, LUL will only unwind through the first 32KB of
stack.  This is mostly harmless, since most stacks are smaller than 4KB, per
measurements today, but occasionally they go above 32KB, causing unwinding to
stop prematurely.

The proposal is to set the max size to 128KB, and to document in comments the
rationale for copying the stack and unwinding, rather than unwinding in place.
128KB is big enough for all stacks observed in several minutes of profiling all
threads at 1KHz, loading the Wikipedia Obama page and also techcrunch.com.
Summary: LUL: increase size of unwound stack to 128k → LUL: increase size of unwound stack to 160k
For reasons related to the architecture of the Gecko Profiler in previous years,
which are no longer relevant, LUL will only unwind through the first 32KB of
stack.  This is mostly harmless, since most stacks are smaller than 4KB, per
measurements today, but occasionally they go above 32KB, causing unwinding to
stop prematurely.

This patch changes the max size to 160KB, and documents the rationale for
copying the stack and unwinding, rather than unwinding in place.  160KB is big
enough for all stacks observed in several minutes of profiling all threads at
1KHz.
Attachment #8858499 - Flags: review?(nfroyd)
Assignee: nobody → jseward
Status: NEW → ASSIGNED
Comment on attachment 8858499 [details] [diff] [review]
LUL: increase size of unwound stack to 160k

Review of attachment 8858499 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, I guess.  I don't see a good way to split the comments up and still make them informative in both locations.

::: tools/profiler/lul/LulMain.h
@@ +160,5 @@
>  
> +// The maximum number of bytes in a stack snapshot.  This can be increased if
> +// necessary, but larger values cause DoNativeBacktrace's frame to become
> +// excessively large.  In practice 160k seems to be enough to get good
> +// backtraces on x86_64-linux.

"larger values cause DoNativeBacktrace's frame to become excessively large"...oh really? :)

How about:

"The maximum number of bytes in a stack snapshot.  This value can be increased if necessary, but testing showed that 160k is enough to obtain good backtraces on x86-64 Linux.  Most backtraces fit comfortably into 4-8k of stack space, but we do have some very deep stacks occasionally.  Please see the comments in DoNativeBacktrace as to why it's OK to have this value be so large."

I feel like some of the comment you wrote in platform.cpp could be profitably moved here, but the context there is important as well.  At least providing a pointer here will help curious people find the rationale behind the size we're allocating and the strategy we use for stack unwinding.
Attachment #8858499 - Flags: review?(nfroyd) → review+
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0510315b5641
LUL: increase size of unwound stack to 160k.  r=froydnj.
https://hg.mozilla.org/mozilla-central/rev/0510315b5641
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.