Closed Bug 1011132 Opened 5 years ago Closed 5 years ago

CYCLE_COLLECTOR_TIME_BETWEEN is computed incorrectly

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

Details

(Whiteboard: [qa-])

Attachments

(1 file)

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.
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.
Assignee: nobody → continuation
Summary: Investigate CYCLE_COLLECTOR_TIME_BETWEEN → CYCLE_COLLECTOR_TIME_BETWEEN is computed incorrectly
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.
Attachment #8423320 - Flags: review?(bugs)
Attachment #8423320 - Flags: review?(bugs) → review+
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 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
Attachment #8423320 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/175ef803c920
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8423320 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-testsuite?
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.