1011132 - CYCLE_COLLECTOR_TIME_BETWEEN is computed incorrectly

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

The numbers I'm seeing for this on telemetry and locally are really odd.  The description says that it is in seconds, but actually it is in ms.  That's the first problem.  Telemetry says the mean is around 13k, which is on the order of what I'd expect, but the median is around 280, which is ridiculously short.  This weirdness was around before ICC, too.

My local telemetry says 120, which is pretty bad, but we're definitely not running CC every 120ms.  So where is that coming from?

We are running the CC almost immediately after the GC, judging by the browser console, which is bad.  I think Olli fixed that at some point, so I'm not sure why we have regressed.

Comment 1

[Andrew McCreight [:mccr8]]

I'm seeing 120 locally because I think that's the top bucket.  The buckets at the bottom of the telemetry page make more sense: everything is 120.  So let's just divide this by 1000 and then see what happens.  I have no idea why the top chart is showing anything else.  I think this should also get backported to at least Aurora so we have some more useful data about what non-ICC looks like.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Fix CYCLE_COLLECTOR_TIME_BETWEEN telemetry.

I'm not sure when this broke, but I assume I broke it at some point during the ICC reworking, so it has been this way for a while.

Comment 3

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca499636ff9e

Comment 4

[Ryan VanderMeulen [:RyanVM]]

This patch had the misfortune of landing in the middle of a large bustage pileup on inbound that led to me having to revert to a last-good cset in lieu of waiting 3-4 hours for green instead. Unfortunately, that means this was backed out along with the others. If this was confirmed green on Try, it can re-land at your convenience.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2a6f7785a2

Comment 5

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=7953cd2846d3

https://hg.mozilla.org/integration/mozilla-inbound/rev/175ef803c920

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8423320 [details] [diff] [review]
Fix CYCLE_COLLECTOR_TIME_BETWEEN telemetry.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): something 6 months or so old
User impact if declined: Incremental cycle collection has been turned on in 32, and I would like to compare its behavior to non-incremental CC in 31 to ensure there is no large regression, but this particular telemetry measure is broken.  Regressions could cause jankiness for users.
Testing completed (on m-c, etc.): local testing, on inbound
Risk to taking this patch (and alternatives if risky): extremely low, it just divides a number we report to telemetry by 1000.
String or IDL/UUID changes made by this patch: none

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/175ef803c920

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b054afbbf6b