1365854 - Remove FRAME_LABEL_COPY

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Nicholas Nethercote [inactive]]

Now that ProfileEntry has both a label and a dynamic string, we don't need to allow label to be dynamic, which means we can remove FRAME_LABEL_COPY.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove FRAME_LABEL_COPY

ProfileEntry has |string|, which can be static or dynamic, and |dynamicString|.
If |string| is dynamic, the FRAME_LABEL_COPY flag must be set, and it will be
copied into profiler output.

But there is only one place that uses dynamic |string| values, in SpiderMonkey.
And that place doesn't use |dynamicString|. So this patch changes that place to
use an empty |string| and put the old dynamic |string| value in
|dynamicString|. This in turn removes the need for FRAME_LABEL_COPY.

One minor wrinkle is that when |dynamicString| is used the old code put a space
between |string| and |dynamicString|. The new code omits the space if |string|
is empty.

The patch also renames ProfileEntry::string as ProfileEntry::label_, which
better matches how it's used.

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8868897 [details] [diff] [review]
Remove FRAME_LABEL_COPY

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

I like it!

::: js/public/ProfilingStack.h
@@ +42,3 @@
>  
> +    // An additional descriptive string of this entry which is combined with
> +    // |label_| in profiler output. Need not (should not!) be static. Can be

I'm taking issue with the "(should not!)" part. For example, the GC reason at http://searchfox.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#1184 picks one of several static string candidates, and that's a perfectly acceptable use of PROFILER_LABEL_DYNAMIC in my opinion.

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8868897 [details] [diff] [review]
Remove FRAME_LABEL_COPY

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

Nice simplification.

::: js/public/ProfilingStack.h
@@ +42,3 @@
>  
> +    // An additional descriptive string of this entry which is combined with
> +    // |label_| in profiler output. Need not (should not!) be static. Can be

Yeah, agree with mstange here. "additional" is fine -- no need to constrain it imo. The dynamic part is really that the profiler can't assume it's static, not that it must be dynamic. Might warrant a rename to "maybeDescription"? Any name is fine with me though.

Comment 4

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/768e500ad190a4de4ba23899e0a0a550e785a4d3
Bug 1365854 - Remove FRAME_LABEL_COPY. r=mstange,shu.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/768e500ad190