Closed Bug 1308116 Opened 3 years ago Closed 3 years ago

Improve telemetry for non-incremental GCs

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

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.
Also, we should add telemetry for how often and why we repeat GCs in GCRuntime::collect.
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.
Assignee: nobody → jcoppeard
Attachment #8800740 - Flags: review?(sphink)
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 ;
Attachment #8800740 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b89b2c0f03de
Improve GC telemetry for non-incremental GCs r=sfink
https://hg.mozilla.org/mozilla-central/rev/b89b2c0f03de
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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
Flags: needinfo?(jcoppeard)
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.
Flags: needinfo?(jcoppeard)
Attachment #8800740 - Flags: feedback?(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": ["dev-telemetry-gc-alerts@mozilla.org"],
> +    "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?
Attachment #8800740 - Flags: feedback?(francois)
Flags: needinfo?(jcoppeard)
(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.
Flags: needinfo?(jcoppeard)
datareview+
You need to log in before you can comment on or make changes to this bug.