Closed Bug 1368915 Opened 3 years ago Closed 3 years ago

A few profiler clean-ups

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(5 files)

No description provided.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
This increases naming consistency. The remaining aStartTime parameters within
the profiler refer to a different start time than the process start time.
Attachment #8874277 - Flags: review?(mstange)
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.
Attachment #8874278 - Flags: review?(mstange)
The previous patch reduced the maximum native stack size to 256. The same size
seems reasonable for JS and pseudo stacks.
Attachment #8874279 - Flags: review?(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.
> 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?
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.
So what sizes would you suggest for the three frames?
Attachment #8874275 - Flags: review?(mstange) → review+
Attachment #8874276 - Flags: review?(mstange) → review+
Attachment #8874277 - Flags: review?(mstange) → review+
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.
Attachment #8874278 - Flags: review?(mstange) → review+
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
Attachment #8874279 - Flags: review?(mstange) → review+
I suggest using the same number for all of them.
You need to log in before you can comment on or make changes to this bug.