1368915 - A few profiler clean-ups

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Rename a parameter of locked_register_thread()

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove unnecessary mozilla:: qualifiers in plaform.cpp

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Rename aStartTime parameters as aProcessStartTime where appropriate

This increases naming consistency. The remaining aStartTime parameters within
the profiler refer to a different start time than the process start time.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Clean up NativeStack

This patch puts the arrays inside NativeStack, gives NativeStack a constructor,
and renames its fields using "mFoo" form.

The patch also moves MAX_NATIVE_FRAMES from the LUL-only code and applies it
globally. This reduces the max native frame count from 1000 to 256 on the
non-LUL platforms, which seems like plenty, and makes things consistent.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Reduce maximum JS and pseudo stack sizes

The previous patch reduced the maximum native stack size to 256. The same size
seems reasonable for JS and pseudo stacks.

Comment 6

[Markus Stange [:mstange]]

I'm not convinced that 256 is enough. Stacks that involve JavaScript can get quite deep.

For example, here are two stacks in a profile of perf.html loading: https://perfht.ml/2qONs0u
They're so deep that they get cut off by the profile tree's hardcoded maximum width. Using right click + copy stack I was able to find out that the deeper one of the two was 554 frames deep. That's the combined depth of C++ + Pseudo + JS frames.

If I understand things correctly, the MAX_NATIVE_FRAMES limit starts counting at the leaf, whereas the other two start counting at the root. (Is that right?) In the latter case I actually care less about loss of information than in the former. If the frames near the root are intact, the regular tree still makes sense, but if they're truncated, that tree has many useless extra roots that start with an arbitrary frame.

I think 1000 (or 1024, if you will) is not such a bad choice.

Comment 7

[Nicholas Nethercote [inactive]]

> I think 1000 (or 1024, if you will) is not such a bad choice.

Ok. Is that just for the JS stack? Is 256 ok for the native stack and pseudostack?

Comment 8

[Markus Stange [:mstange]]

The native stack contains at least one frame per JS frame, so that's the one that needs the most frames.
And I think interpreter JS frames still push pseudostack labels, so the pseudostack needs to be at least size of the JS stack.

Comment 9

[Nicholas Nethercote [inactive]]

So what sizes would you suggest for the three frames?

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8874278 [details] [diff] [review]
(part 4) - Clean up NativeStack

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

::: tools/profiler/core/platform.cpp
@@ +806,5 @@
>  }
>  
> +// The maximum number of native frames obtained. Setting it too high risks the
> +// unwinder wasting a lot of time looping on corrupted stacks.
> +static const size_t MAX_NATIVE_FRAMES = 256;

Please set this to 1000.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8874279 [details] [diff] [review]
(part 5) - Reduce maximum JS and pseudo stack sizes

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

::: js/public/ProfilingStack.h
@@ +258,5 @@
>      PseudoStack(const PseudoStack&) = delete;
>      void operator=(const PseudoStack&) = delete;
>  
>    public:
> +    static const uint32_t MaxEntries = 256;

1000 here as well

::: tools/profiler/core/platform.cpp
@@ +812,2 @@
>  static const size_t MAX_NATIVE_FRAMES = 256;
> +static const size_t MAX_JS_FRAMES     = 256;

and here

Comment 12

[Markus Stange [:mstange]]

I suggest using the same number for all of them.

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/827b05e1b8f77ff34829b7002a582363b41c9f92
Bug 1368915 (part 1) - Rename a parameter of locked_register_thread(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2019c06e297c20de7afcd05716b6298ee2ea36f2
Bug 1368915 (part 2) - Remove unnecessary mozilla:: qualifiers in plaform.cpp. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d5a07cede0aa1d2de6fdf13b7f522aa4a1cc22
Bug 1368915 (part 3) - Rename aStartTime parameters as aProcessStartTime where appropriate. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f636eeb949d07746a412fbf327ce4e4015f2aad
Bug 1368915 (part 4) - Clean up NativeStack. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0145ab10f5cc62bdf5cee9453be305809e5f7840
Bug 1368915 (part 5) - Introduce MAX_JS_FRAMES. r=mstange.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/827b05e1b8f7
https://hg.mozilla.org/mozilla-central/rev/2019c06e297c
https://hg.mozilla.org/mozilla-central/rev/e7d5a07cede0
https://hg.mozilla.org/mozilla-central/rev/2f636eeb949d
https://hg.mozilla.org/mozilla-central/rev/0145ab10f5cc