1978820 - GCREASONS values don't match GC reasons in metrics.yaml

Status: RESOLVED FIXED

(Core :: JavaScript: GC, task)

Description

[Jan de Mooij [:jandem]]

Value 12 is missing here so the list of GC reasons in xpconnect/metrics.yaml after that point are off by one. XPConnect has static assertions to check LAST_FIREFOX_REASON matches the number of reasons in metrics.yaml but these assertions need a +1 to be correct.

I noticed this while adding a new GC reason for bug 1977367.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1978820 - Add missing GC reason 12 to GCREASONS list and metrics.yaml. r?jonco!

The static assertions need a +1 because LAST_FIREFOX_REASON is the PREPARE_FOR_PAGELOAD
reason and Glean's e__Other__ value comes after that.
See https://searchfox.org/mozilla-central/source/__GENERATED__/toolkit/components/glean/JsXpconnectMetrics.h#409

Comment 2

[Jon Coppeard (:jonco)]

I'm a bit worried that this means there is an off by one somewhere further along the chain though, because the labels in Glam look plausible with INTER_SLICE_GC being the most common reason.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #2)

I'm a bit worried that this means there is an off by one somewhere further along the chain though, because the labels in Glam look plausible with INTER_SLICE_GC being the most common reason.

That is surprising. I'll see if I can get more data from a local build.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)

(In reply to Jon Coppeard (:jonco) from comment #2)

I'm a bit worried that this means there is an off by one somewhere further along the chain though, because the labels in Glam look plausible with INTER_SLICE_GC being the most common reason.

That is surprising. I'll see if I can get more data from a local build.

The code converts the GCReason to a string using the JS GCReason definition. Jon and I think that maybe it all just works because of that.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/abddf68a84e0
https://hg.mozilla.org/integration/autoland/rev/cf30c564cdec
Add missing GC reason 12 to GCREASONS list and metrics.yaml. r=jonco

Comment 6

[Atila Butkovits]

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