1113325 - The InterSlice GC timer can cause long GC pauses when painting

Status: RESOLVED DUPLICATE of bug 1367905

(Core :: JavaScript: GC, defect)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

In DOMGCSliceCallback, we kick off a timer on a 100ms delay (NS_INTERSLICE_GC_DELAY) that triggers an incremental slice if the GC has more work to do. This slice is given a budget of 40ms (NS_INTERSLICE_GC_DELAY) [2], which was deemed good enough to not be noticeably janky.

However, we want shorter slices when painting, so that we don't start dropping frames. To this end, the refresh driver sends NotifyDidPaint notifications [3], which trigger a single 10ms slice (javascript.options.mem.gc_incremental_slice_ms, 30ms on B2G) each frame [4]. 

However, the InterSlice timer knows nothing about these notifications, so if the incremental GC is still in progress when the timer fires, we can randomly get a 40ms slice.

I propose we cancel any existing InterSlice timer at nsXPConnect::NotifyDidPaint(). If we're painting, 40ms is definitely not okay, and even if we had a principled way to dynamically lower the slice time, we still don't want more than one of these slices per frame. The only worry is that with the timer gone, what happens if we stop painting and the incremental GC is still going? Maybe we fire off another timer, but I don't know the code well enough. Anyway, one-liner patch coming up.

[1] http://hg.mozilla.org/mozilla-central/annotate/0e441ff66c5e/dom/base/nsJSEnvironment.cpp#l2374
[2] http://hg.mozilla.org/mozilla-central/annotate/0e441ff66c5e/dom/base/nsJSEnvironment.cpp#l1964
[3] http://hg.mozilla.org/mozilla-central/annotate/0e441ff66c5e/layout/base/nsRefreshDriver.cpp#l1384
[4] http://hg.mozilla.org/mozilla-central/annotate/0e441ff66c5e/js/src/jsgc.cpp#l6354

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Cancel InterSlice GC timer when we get a NotifyDidPaint event.

This seems to work in local testing, but I don't have a good testcase to really put it through its paces. Running MemBench [1], I got 19 GCs from the InterSlice timer without the patch, and 1 GC with - but that's not much of a statistical sample.

[1] http://gregor-wagner.com/tmp/mem

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7758e5d3f451

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8538715 [details] [diff] [review]
Cancel InterSlice GC timer when we get a NotifyDidPaint event.

Review of attachment 8538715 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ Thank you! I believe that this is correct -- e.g. the slice will still get rescheduled at the end of GC so we don't lose it permanently if we stop animating. I'm going to forward the review to Olli to verify that my understanding is correct.

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #0)
> a budget of 40ms (NS_INTERSLICE_GC_DELAY)

By this I meant NS_INTERSLICE_GC_BUDGET of course ;)

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

This seems to be tripping the assert at [1] because sInterSliceGCTimer is null, so it's no longer taking the early return. Olli, was this masking a pre-existing problem or is this breaking an invariant that the CC relies on?

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

Forgot my reference:

[1] http://hg.mozilla.org/mozilla-central/annotate/0e441ff66c5e/dom/base/nsJSEnvironment.cpp#l2140

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

It breaks an invariant. So you probably want to add a boolean or so to track whether iGC is ongoing.

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: v2 - Cancel InterSlice GC timer when we get a NotifyDidPaint event.

We already have sCCLockedOut while iGC is in progress, so this reuses it. We return early when sCCLockedOut but !sInterSliceGCTimer, since the timer is only cancelled (after this patch) if the refresh driver is driving incremental GC.

I also adjusted nsJSContext::PokeGC with the same reasoning. I know Andrew is rejiggering how that stuff works in bug 1110928; I don't know if the early return still makes sense after that.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

It is not quite clear to me why
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp?rev=158a3009efa2&mark=2370-2371#2333 isn't enough.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8538849 [details] [diff] [review]
v2 - Cancel InterSlice GC timer when we get a NotifyDidPaint event.

So, please explain why we should kill the timer in NotifyDidPaint.
But as far as I see, this shouldn't be needed.

(re-ask review if this is needed.)

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Olli Pettay [:smaug] from comment #9)
> It is not quite clear to me why
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.
> cpp?rev=158a3009efa2&mark=2370-2371#2333 isn't enough.

> So, please explain why we should kill the timer in NotifyDidPaint.
> But as far as I see, this shouldn't be needed.

I think you're right: this is the crucial interaction that I was missing before.

So the innocuous sounding js::gcstats::Statistics has a sliceCallback member, which is called in Statistics::beginSlice() and Statistics::endSlice() (called by AutoGCSlice::AutoGCSlice() and AutoGCSlice::~AutoGCSlice() respectively). This callback ensures that incremental GCs are actually finished by scheduling the InterSlice GC timer, and this will also *restart* the timer if we do a slice before it fires. There's no bug here.

Having said that, the logic in GCRuntime::notifyDidPaint() seems kind of backwards to me.

1) We only do a slice if !interFrameGC, then reset interFrameGC at the end (we unconditionally set interFrameGC to true at the start of GCRuntime::gcCycle()). We just painted, so isn't this the best time to do a slice? Since we punt until the next frame regardless of how long ago the last slice happened, this could also cause us to hit the InterSlice GC timer even though we started painting.

It would make more sense to me if we had a GCRuntime::didPaintGC which prevents further non-REFRESH_FRAME slices during a frame, and a GCRuntime::notifyDidNotPaint() that resets it (called during uneventful refresh driver ticks).

2) GCRuntime::notifyDidPaint() can only finish *ongoing* incremental GCs. If no iGC is ongoing, it will do nothing. It seems to me that since we're in the neighborhood, we should check if there's any point in starting a new GC now, rather than waiting for more work at an inopportune time. Perhaps it could call something like maybeGC, checking for any zones that need it? That doesn't sound like something for this bug, though.

Comment 12

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Change NotifyDidPaint logic to work immediately instead of with a frame delay.

Something like this. A few questions:

1) Is this API too much of a footgun? We only use it from the refresh driver and it's marked as internal, but forgetting to call NotifyDidNotPaint would be bad.
2) Would it be better to give NotifyDidPaint a boolean parameter instead? (true for did paint, false for did not) An enum would be clearer but this goes through several layers so I wouldn't know where to put it.
3) Is suppressing collection like this okay? It should very effectively prevent slices from other origins, but it doesn't prevent PrepareForIncrementalGC or anything.
4) Should we think about what to do in the presence of multiple refresh drivers? This is a pre-existing problem, but if there's more than one refresh driver each one will tell us if it painted or not. Undesirable now, but getting a NotifyDidNotPaint notification because one of the refresh drivers happened to not paint would disable the slice suppression.

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 8539296 [details] [diff] [review]
Change NotifyDidPaint logic to work immediately instead of with a frame delay.

Review of attachment 8539296 [details] [diff] [review]:
-----------------------------------------------------------------

I like the state management of "are we animating" much more, but the new heuristic is (1) a bit too strong and (2) scatters yet another random heuristic into the code. If possible, I'd like to centralize the existing logic before we start sowing even more confusion.

::: js/src/gc/GCRuntime.h
@@ +795,2 @@
>       */
> +    mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> didPaintGC;

I don't think this is ever actually accessed from off the main thread? In the case where we set it, we run a GC immediately, so that must be on the main thread. And, other than that, we only ever access it from inside gcCycle.

::: js/src/jsgc.cpp
@@ +6249,5 @@
>      if (rt->mainThread.suppressGC)
>          return;
>  
> +    if (didPaintGC && gckind == GC_NORMAL && reason != JS::gcreason::REFRESH_FRAME)
> +        return;

This goes too far: it will stop us from running LAST_DITCH GC when we're actually out of memory and /need/ to do a complete GC before proceeding.

Comment 14

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Terrence Cole [:terrence] from comment #13)
> If possible, I'd like to centralize the existing
> logic before we start sowing even more confusion.

Makes sense. I'm not really sure where to start, myself, but at least I'm getting a better understanding of how this currently works.

> > +    mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> didPaintGC;
> 
> I don't think this is ever actually accessed from off the main thread? In
> the case where we set it, we run a GC immediately, so that must be on the
> main thread.

Good point.

> > +    if (didPaintGC && gckind == GC_NORMAL && reason != JS::gcreason::REFRESH_FRAME)
> > +        return;
> 
> This goes too far: it will stop us from running LAST_DITCH GC when we're
> actually out of memory and /need/ to do a complete GC before proceeding.

I meant to add a check for sliceBudget.isUnlimited(), but forgot. I originally had this in gcSlice, where you know it's incremental (you can get a GC_SHRINK, but that won't match). This part feels kind of icky in any case; maybe we can turn this into an assertion instead once the scheduling is tidied up.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 16

[Steve Fink [:sfink] [:s:]]

I believe that the switch over to idle-based scheduling takes care of this—if we're animating, we won't be handed these long budgets anymore. The refresh driver will determine the size of the budget, at least until the GC gets impatient and starts lengthening slices in order to finish up.