Add telemetry for nursery promotion rate.

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
9 months ago

People

(Reporter: jonco, Assigned: allstars.chh, Mentored)

Tracking

({good-first-bug})

61 Branch
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

We should add telemetry to track the proportion of nursery objects that we tenure when we collect the nursery.

Nursery telemetry is handled here:

https://searchfox.org/mozilla-central/source/js/src/gc/Nursery.cpp#827
Assignee: nobody → allstars.chh
Status: NEW → ASSIGNED
Comment on attachment 9023588 [details] [diff] [review]
0001-Bug-1485299-Add-telemethy-for-nursery-promotion-rate.patch

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

Hi Jonco
Can you just quickly go through this to see if this is the information we'd like to telemetry?
Thanks
Attachment #9023588 - Flags: feedback?(jcoppeard)
Comment on attachment 9023588 [details] [diff] [review]
0001-Bug-1485299-Add-telemethy-for-nursery-promotion-rate.patch

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

Thanks Yoshi, this looks good to me!

::: js/src/jsfriendapi.h
@@ +156,5 @@
>      JS_TELEMETRY_GC_NURSERY_BYTES,
>      JS_TELEMETRY_GC_PRETENURE_COUNT,
>      JS_TELEMETRY_PRIVILEGED_PARSER_COMPILE_LAZY_AFTER_MS,
>      JS_TELEMETRY_WEB_PARSER_COMPILE_LAZY_AFTER_MS,
> +    JS_TELEMETRY_GC_NURSERY_PROMOTION_RATE,

nit: can you put this with the rest of the GC ones?

::: toolkit/components/telemetry/Histograms.json
@@ +822,5 @@
> +    "alert_emails": ["dev-telemetry-gc-alerts@mozilla.org"],
> +    "expires_in_version": "never",
> +    "kind": "linear",
> +    "high": 100,
> +    "n_buckets": 99,

It doesn't matter too much, but why 99 buckets?
Attachment #9023588 - Flags: feedback?(jcoppeard) → feedback+
Comment on attachment 9023588 [details] [diff] [review]
0001-Bug-1485299-Add-telemethy-for-nursery-promotion-rate.patch

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

::: js/src/jsfriendapi.h
@@ +156,5 @@
>      JS_TELEMETRY_GC_NURSERY_BYTES,
>      JS_TELEMETRY_GC_PRETENURE_COUNT,
>      JS_TELEMETRY_PRIVILEGED_PARSER_COMPILE_LAZY_AFTER_MS,
>      JS_TELEMETRY_WEB_PARSER_COMPILE_LAZY_AFTER_MS,
> +    JS_TELEMETRY_GC_NURSERY_PROMOTION_RATE,

Will do.

::: toolkit/components/telemetry/Histograms.json
@@ +822,5 @@
> +    "alert_emails": ["dev-telemetry-gc-alerts@mozilla.org"],
> +    "expires_in_version": "never",
> +    "kind": "linear",
> +    "high": 100,
> +    "n_buckets": 99,

Originally I set it to 100, but it triggered a static assert,

In file included from /home/allstars/src/gecko_icecc/toolkit/components/telemetry/core/TelemetryHistogram.cpp:255:
 7:13.34 /home/allstars/src/gecko_icecc/obj-x86_64-pc-linux-gnu/toolkit/components/telemetry/TelemetryHistogramData.inc:6328:1: error: static_assert failed "high must be > number of buckets for GC_NURSERY_PROMOTION_RATE; you may want an enumerated histogram"
 7:13.34 static_assert(100 > 100, "high must be > number of buckets for GC_NURSERY_PROMOTION_RATE; you may want an enumerated histogram");

As you suggested on IRC, I'll change it to 50.
Attachment #9023659 - Flags: review?(jcoppeard)
Attachment #9023660 - Flags: feedback?(jcoppeard)
Comment on attachment 9023659 [details] [diff] [review]
Add telemetry for GC_NURSERY_PROMOTION_RATE

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

Looks great.

::: toolkit/components/telemetry/Histograms.json
@@ +824,5 @@
> +    "kind": "linear",
> +    "high": 100,
> +    "n_buckets": 50,
> +    "bug_numbers": [1485299],
> +    "description": "The percentage of nursery objects get promoted into tenured heap."

nit: "The percentage of nursery objects that were promoted to the tenured heap"
Attachment #9023659 - Flags: review?(jcoppeard) → review+
Comment on attachment 9023660 [details]
Data review request

This is fine.  I'm going to add to it in a couple of places and put it up for review.
Attachment #9023660 - Flags: feedback?(jcoppeard) → feedback+
Requesting data review.
Attachment #9023677 - Flags: review?(chutten)
Attachment #9023660 - Attachment is obsolete: true
Comment on attachment 9023677 [details]
data_collection_review.txt

Preliminary notes:

  We need an individual's name for the answer to question 4 for permanent measures. I'm going to presume that :jonco will step up for that. Ideally also there would be an individual's email address in alert_emails (it's a list, so you can put the list and your own address on it at the same time).

  For permanent measures it is usually a good idea to have automated tests to ensure no one else's future patch breaks your collection.

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. Standard Telemetry mechanisms apply.

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. Standard Telemetry mechanisms apply.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, :jonco will.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

    Is the data collection request for default-on or default-off?

Default on. All channels.

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. Permanent collection.

---
Result: datareview+
Attachment #9023677 - Flags: review?(chutten) → review+
(In reply to Chris H-C :chutten from comment #10)
>   We need an individual's name for the answer to question 4 for permanent
> measures. I'm going to presume that :jonco will step up for that.

Yes, I'll do this.  Cheers!
Yoshi, please add my email address to the list in Histograms.json.  Then I think we're ready to land.
updated Histgrams.json
- added jcoppeard@mozilla.com in alert_emails
- updated description in comment 7
Attachment #9023659 - Attachment is obsolete: true
Attachment #9023851 - Flags: review+
Updated commit message.
Attachment #9023851 - Attachment is obsolete: true
Attachment #9023879 - Flags: review+
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c160322e846
Add telemethy for nursery promotion rate. r=jonco, data-review=chutten
https://hg.mozilla.org/mozilla-central/rev/3c160322e846
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.