1485299 - Add telemetry for nursery promotion rate.

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

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

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: 0001-Bug-1485299-Add-telemethy-for-nursery-promotion-rate.patch

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 3

[Jon Coppeard (:jonco)]

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": ["[e-mail]"],
> +    "expires_in_version": "never",
> +    "kind": "linear",
> +    "high": 100,
> +    "n_buckets": 99,

It doesn't matter too much, but why 99 buckets?

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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": ["[e-mail]"],
> +    "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.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Add telemetry for GC_NURSERY_PROMOTION_RATE

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Data review request

Comment 7

[Jon Coppeard (:jonco)]

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"

Comment 8

[Jon Coppeard (:jonco)]

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.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: data_collection_review.txt

Requesting data review.

Comment 10

[Chris H-C :chutten]

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+

Comment 11

[Jon Coppeard (:jonco)]

(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!

Comment 12

[Jon Coppeard (:jonco)]

Yoshi, please add my email address to the list in Histograms.json.  Then I think we're ready to land.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Add telemetry for GC_NURSERY_PROMOTION_RATE. v2

updated Histgrams.json
- added [e-mail] in alert_emails
- updated description in comment 7

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Add telemetry for GC_NURSERY_PROMOTION_RATE. v2.1

Updated commit message.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c160322e846
Add telemethy for nursery promotion rate. r=jonco, data-review=chutten

Comment 16

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3c160322e846