1166524 - Provide nice labels and descriptions for GC reasons in the marker's detail view

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

For example:

  reason: ALLOC_TRIGGER

Could get L10N'd into something like this:

  Reason: Allocation Trigger
  Description: GC was triggered because there was too much allocation. Consider finding ways to avoid unnecessary allocations.

The tricky thing is keeping up-to-date and in sync with the actual GC reasons, while not losing data about old GC reasons, because we might be remote profiling an older gecko/spidermonkey.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

So there are probably two components here -- one is labeling reason/non-incremental reasons to something nice and ideally understandable to the average web developer. "Allocation Trigger" still doesn't really tell me what happened (did I trigger a number of allocations over some threshold? Did I allocate a very large object?).

If we can translate the reasons into a digestible 2-3 word description, that'd be great. If not, then the more detailed descriptions would be more actionable (but obviously more work).

Is there any value to the platform in having the non "translated" forms of the reasons listed, or would they be all pretty analogous to the platform reason?

w/r/t profiling older geckos: how often would these reasons change? In that case, would it be possible to enforce new reasons (if ALLOC_TRIGGER means something else in Fx45, can we call it ALLOC_TRIGGER_2, for lack of creativity and knowledge?), so we will still have a mapping from reason to description that's accurate for all geckos.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #1)
> w/r/t profiling older geckos: how often would these reasons change? In that
> case, would it be possible to enforce new reasons (if ALLOC_TRIGGER means
> something else in Fx45, can we call it ALLOC_TRIGGER_2, for lack of
> creativity and knowledge?), so we will still have a mapping from reason to
> description that's accurate for all geckos.

I just meant the addition/removal of reasons -- I'm not sure how we would handle the same reason changing semantically.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Removal can be done whenever, worst case we have extra string mappings that aren't used currently (but would work in older geckos). This would all handle scenarios of no mappings existing (in which case, no nice description and the raw "reason" from the platform, which would either be sufficient or be an indicator we need to add something). Markers are now pretty robust and easy to customize displays and handle fallbacks FWIW

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

For the JIT Opt stuff, something similar is happening in bug 1145442 @ https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JIT_Optimization_Strategies

We should probably do something similar for this.

Sending this fitzgen's way

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Provide short labels and longer descriptions for GC reasons

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

There were a couple reasons that seemed similar enough that if you aren't a gecko hacker, you wouldn't gain anything from knowing that they were different. Because of that, I used the same label/description for those reasons.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8643304 [details] [diff] [review]
Provide short labels and longer descriptions for GC reasons

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

With bug 1163763 landed, these should be moved to marker.properties, and the root of each string should be `marker` instead of `profiler`.

Many descriptions have unclear pronouns, or are too casual (we shouldn't have any usage of "We" in any of the descriptions). All in all this is amazing; can't wait to get this landed.

::: browser/locales/en-US/chrome/browser/devtools/profiler.properties
@@ +195,5 @@
> +
> +# LOCALIZATION NOTE (profiler.gcreason.description.*):
> +# These strings are used to give an expanded description of why a GC occurred.
> +profiler.gcreason.description.API=There was an API call to force garbage collection.
> +profiler.gcreason.description.EAGER_ALLOC_TRIGGER=We returned to the event loop and there were enough bytes allocated since the last GC that a new GC cycle was triggered.

A lot of these descriptions have "We" in it -- I think it'd make more sense without that pronoun -- who's "We"? Firefox? The web content? Garbage collector? "When returning to the event loop, there were enough bytes..."

@@ +201,5 @@
> +profiler.gcreason.description.DESTROY_CONTEXT=We destroyed a JavaScript runtime or context, and this was the final garbage collection before shutting down.
> +profiler.gcreason.description.LAST_DITCH=We attempted to allocate, but there was no memory available. Doing a full compacting garbage collection as an attempt to free up memory for the allocation.
> +profiler.gcreason.description.TOO_MUCH_MALLOC=JavaScript allocated too many bytes, and forced a garbage collection.
> +profiler.gcreason.description.ALLOC_TRIGGER=JavaScript allocated too many times, and forced a garbage collection.
> +profiler.gcreason.description.DEBUG_GC=GC due to Zeal debug settings.

Zeal? Looks like a debug thing, so doubt this needs further clarification

@@ +210,5 @@
> +profiler.gcreason.description.FULL_STORE_BUFFER=There were too many properties on tenured objects whose value was an object in the nursery.
> +profiler.gcreason.description.SHARED_MEMORY_LIMIT=A large allocation was requested, but there was not enough memory.
> +profiler.gcreason.description.PERIODIC_FULL_GC=We returned to the event loop, and it has been a relatively long time since we performed a garbage collection.
> +profiler.gcreason.description.INCREMENTAL_TOO_SLOW=A full, non-incremental garbage collection was triggered because there was a faster rate of allocations than the existing incremental garbage collection cycle could keep up with.
> +profiler.gcreason.description.DOM_WINDOW_UTILS=The user was inactive for a long time. Took the opportunity to perform GC when it was unlikely to be noticed.

Needs a subject -- what took the opportunity? "SUBJECT took the opportunity...", Firefox, Spidermonkey, etc.

@@ +211,5 @@
> +profiler.gcreason.description.SHARED_MEMORY_LIMIT=A large allocation was requested, but there was not enough memory.
> +profiler.gcreason.description.PERIODIC_FULL_GC=We returned to the event loop, and it has been a relatively long time since we performed a garbage collection.
> +profiler.gcreason.description.INCREMENTAL_TOO_SLOW=A full, non-incremental garbage collection was triggered because there was a faster rate of allocations than the existing incremental garbage collection cycle could keep up with.
> +profiler.gcreason.description.DOM_WINDOW_UTILS=The user was inactive for a long time. Took the opportunity to perform GC when it was unlikely to be noticed.
> +profiler.gcreason.description.COMPONENT_UTILS=Someone called Components.utils.forceGC() to force a garbage collection.

"Components.utils.forceGC() was called to force a garbage collection."

@@ +215,5 @@
> +profiler.gcreason.description.COMPONENT_UTILS=Someone called Components.utils.forceGC() to force a garbage collection.
> +profiler.gcreason.description.MEM_PRESSURE=There was very low memory available.
> +profiler.gcreason.description.CC_WAITING=The cycle collector required a garbage collection.
> +profiler.gcreason.description.CC_FORCED=The cycle collector required a garbage collection.
> +profiler.gcreason.description.LOAD_END=The webpage finished loading.

s/webpage/document

@@ +223,5 @@
> +profiler.gcreason.description.SET_DOC_SHELL=The page has been navigated to a new document.
> +profiler.gcreason.description.DOM_UTILS=There was an API call to force garbage collection.
> +profiler.gcreason.description.DOM_IPC=Received an inter-process message that requested a garbage collection.
> +profiler.gcreason.description.DOM_WORKER=The worker was idle for a relatively long time.
> +profiler.gcreason.description.INTER_SLICE_GC=It has been relatively long since the last incremental GC slice.

Too casual and unclear -- What is "it" and "relatively long" needs some other noun, "relatively long time"?

@@ +227,5 @@
> +profiler.gcreason.description.INTER_SLICE_GC=It has been relatively long since the last incremental GC slice.
> +profiler.gcreason.description.FULL_GC_TIMER=We returned to the event loop, and it has been a relatively long time since we performed a garbage collection.
> +profiler.gcreason.description.SHUTDOWN_CC=We destroyed a JavaScript runtime or context, and this was the final garbage collection before shutting down.
> +profiler.gcreason.description.FINISH_LARGE_EVALUATE=We finished evaluating a large script, and we performed a GC because it will never be run again.
> +profiler.gcreason.description.USER_INACTIVE=The user was inactive for a long time. Took the opportunity to perform GC when it was unlikely to be noticed.

Same as DOM_WINDOW_UTILS -- should group any descriptions that are similar together as well here.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8643304 [details] [diff] [review]
Provide short labels and longer descriptions for GC reasons

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

Seems fine to me with the changes here applied. I'm going to feedback Steve on this for when he gets back from PTO, I'm sure he'll have better suggestions.

::: browser/locales/en-US/chrome/browser/devtools/profiler.properties
@@ +196,5 @@
> +# LOCALIZATION NOTE (profiler.gcreason.description.*):
> +# These strings are used to give an expanded description of why a GC occurred.
> +profiler.gcreason.description.API=There was an API call to force garbage collection.
> +profiler.gcreason.description.EAGER_ALLOC_TRIGGER=We returned to the event loop and there were enough bytes allocated since the last GC that a new GC cycle was triggered.
> +profiler.gcreason.description.DESTROY_RUNTIME=We destroyed a JavaScript runtime or context, and this was the final garbage collection before shutting down.

I'd replace "we" with "Firefox" in all of these.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Provide short labels and longer descriptions for GC reasons

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8643844 [details] [diff] [review]
Provide short labels and longer descriptions for GC reasons

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

Looks good! Should be in marker.properties (with `marker.gcreason.*` namespace), and other than that and some nits, looks great.

::: browser/locales/en-US/chrome/browser/devtools/performance.properties
@@ +220,5 @@
> +profiler.gcreason.description.DEBUG_GC=GC due to Zeal debug settings.
> +profiler.gcreason.description.COMPARTMENT_REVIVED=A global object that was thought to be dead at the start of the GC cycle was revived by the end of the GC cycle.
> +profiler.gcreason.description.RESET=The active incremental GC cycle was forced to finish immediately.
> +profiler.gcreason.description.OUT_OF_NURSERY=JavaScript allocated enough new objects in the nursery that it became full and triggered a minor GC.
> +profiler.gcreason.description.EVICT_NURSERY=Work needed to be done on the tenured heap, and so the nursery must be empty.

too casual: "Work needed to be done on the tenured heap, requiring an empty nursery" or something

@@ +237,5 @@
> +profiler.gcreason.description.SET_DOC_SHELL=The page has been navigated to a new document.
> +profiler.gcreason.description.DOM_UTILS=There was an API call to force garbage collection.
> +profiler.gcreason.description.DOM_IPC=Received an inter-process message that requested a garbage collection.
> +profiler.gcreason.description.DOM_WORKER=The worker was idle for a relatively long time.
> +profiler.gcreason.description.INTER_SLICE_GC=There has been a relatively long delay since the last incremental GC slice.

s/delay/time

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/816eb5a838eb

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/816eb5a838eb