Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

2 years ago
No description provided.
Assignee

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee

Comment 3

2 years ago
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)
Assignee

Comment 4

2 years ago
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)
Assignee

Comment 5

2 years ago
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.
Assignee

Comment 7

2 years ago
> 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.
Assignee

Comment 9

2 years ago
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.