Reduce our definition for what constitutes a long GC slice

RESOLVED FIXED in Firefox 56



2 years ago
2 years ago


(Reporter: jonco, Assigned: jonco)


55 Branch

Firefox Tracking Flags

(firefox56 fixed)


(Whiteboard: [qf:p3])


(1 attachment, 1 obsolete attachment)



2 years ago
We currently record telemetry for slices that exceeded twice their budget.  We're trying to be much better about keeping to the budget, and a factor of two is really pretty bad.

Let's reduce this.


2 years ago
Whiteboard: [qf] → [qf:p3]

Comment 1

2 years ago
Patch to change the definition of a 'long' slice from twice the budget to one-and-a-half the budget or 5ms whichever is shorter.

This also changes the way slow parallel tasks are detected so that now they are recorded if waiting for a parallel task was the longest phase.
Attachment #8887406 - Flags: review?(sphink)
Comment on attachment 8887406 [details] [diff] [review]

Review of attachment 8887406 [details] [diff] [review]:

::: js/src/gc/Statistics.cpp
@@ +980,5 @@
>              if (budget_ms == runtime->gc.defaultSliceBudget())
>                  runtime->addTelemetry(JS_TELEMETRY_GC_ANIMATION_MS, t(sliceTime));
> +            // Record any phase that goes 1.5 times or 5ms over its budget.
> +            double longSliceThreshold = std::min(1.5 * budget_ms, 5.0);

Heh. I don't think this is what you were thinking of. This is going to report for any slice that goes over 5ms (as in, pretty much all of them.) I believe you wanted

  std::min(1.5 * budget_ms, 5.0 + budget_ms)

Is budget+5ms what we're shooting for? As in, is it as bad for a 50ms idle slice to take 55ms as it is for a 10ms slice to take 15ms?

This long slice definition boils down to "150% of budgets 10ms or less, otherwise 5ms over budget". I didn't think we currently had any budgets less than 10ms (though maybe that's changed with idle scheduling?), so really that's changing from 2 x budget -> 5ms + budget, which is a pretty big change. Not that I think 2xbudget is at all reasonable. But 55ms of a 50ms budget just doesn't feel as bad as 20ms of a 15ms budget (as in, guaranteed frame drop for 60fps.)

@@ +989,5 @@
> +                // If the longest phase was waiting for parallel tasks then
> +                // record the longest task.
> +                if (longest == PhaseKind::JOIN_PARALLEL_TASKS) {
> +                    PhaseKind longestParallel = LongestPhaseSelfTimeInMajorGC(slice.parallelTimes);
> +                    reportLongestPhaseInMajorGC(longestParallel, JS_TELEMETRY_GC_SLOW_TASK);

Very nice change here.
Attachment #8887406 - Flags: review?(sphink)

Comment 3

2 years ago
Updated patch with the error fixed.  We discussed the 5ms issue and Steve gave r+ via Vidyo.
Attachment #8887406 - Attachment is obsolete: true
Attachment #8887958 - Flags: review+

Comment 4

2 years ago
Pushed by
Tighten up our defintion of what constitutes a 'long' slice r=sfink

Comment 5

2 years ago
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.