1308116 - Improve telemetry for non-incremental GCs

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

As mentioned in bug 1308039, we don't report why we do non-incremental GCs via telemetry.  Currently we have a human-readable reason string, but we should replace this with an enum and report the value in our GC telemetry.

Comment 1

[Jon Coppeard (:jonco)]

Also, we should add telemetry for how often and why we repeat GCs in GCRuntime::collect.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1308116-improve-telemetry

This is what I came up with for reset / non-incremental telemetry.

I'm not sure what to do about the repeated GCs right now, possibly that's not so important because we can see RESET slices and we can COMPARTMENT_REVIVED GCs already.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8800740 [details] [diff] [review]
bug1308116-improve-telemetry

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

Oops, I forgot to publish this review.

::: js/src/jsfriendapi.h
@@ +125,3 @@
>      JS_TELEMETRY_GC_INCREMENTAL_DISABLED,
>      JS_TELEMETRY_GC_NON_INCREMENTAL,
> +    JS_TELEMETRY_GC_NON_INCREMENTAL_REASON,

Just to check: you don't need to preserve the existing numbering, right? It looks like there's an indirection via the switch statement in XPCJSContext.cpp, so I think this is fine.

::: js/src/vm/Debugger.cpp
@@ +10973,5 @@
>      auto data = rt->make_unique<GarbageCollectionEvent>(gcNumber);
>      if (!data)
>          return nullptr;
>  
> +    data->nonincrementalReason = stats.nonincrementalReason() ;

extra space added before the ;

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b89b2c0f03de
Improve GC telemetry for non-incremental GCs r=sfink

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b89b2c0f03de

Comment 6

[Alessio Placitelli [:Dexter]]

This seems to have added two new histograms.
Please note that data collection review is needed for that. A list of data stewards, along with some guidelines, is available at:
https://wiki.mozilla.org/Firefox/Data_Collection

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8800740 [details] [diff] [review]
bug1308116-improve-telemetry

(In reply to Alessio Placitelli [:Dexter] from comment #6)
> Please note that data collection review is needed for that. 

Apologies, I did not realise that.

Requesting after the fact review.

Comment 8

[François Marier [:francois]]

Comment on attachment 8800740 [details] [diff] [review]
bug1308116-improve-telemetry

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

::: toolkit/components/telemetry/Histograms.json
@@ +812,5 @@
>      "description": "Was an incremental GC canceled?"
>    },
> +  "GC_RESET_REASON": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",

I notice that most of this telemetry is very old and pre-dates probe expiries. We now have the following telemetry requirement:

"Permanent or longer-term data collection should have a plan for permanent monitoring."
(https://wiki.mozilla.org/Firefox/Data_Collection#Requirements)

Does your team produce reports, have some kind of monitoring in place or otherwise regularly look at this kind of telemetry?

Comment 9

[Jon Coppeard (:jonco)]

(In reply to François Marier [:francois] from comment #8)

> Does your team produce reports, have some kind of monitoring in place or
> otherwise regularly look at this kind of telemetry?

Yes, we monitor this and have a mailing list subscribed to the alerts.

Comment 10

[François Marier [:francois]]

datareview+