1300893 - Frequent slice budget overruns for mark -> sweep transition

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

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

When looking at GC logs, I very often see 40ms slices take 50-60ms because we finish up in 10-30ms, see that we're under our 40ms limit, and then start sweeping. Sweeping starts out with an uninterruptible phase that can easily take 20-30ms.

Actually, that's a bit of a lie. The beginning of sweeping *is* interruptible -- I can see in the code that it's passing the budget around everywhere. But there must be some portion that is not, after it gets past the early stuff?

Looking at my log:

 - I see a long "Sweep Object". Which makes sense, since queueForegroundObjectsForSweep is uninterruptible.
 - This next one is unclear; perhaps "Sweep Compartments", which indeed looks like it has lots of uninterruptible stuff in beginSweepingZoneGroup.
 - The next one only went over by 1ms, so it could just be slop. 
 - The next is "Sweep Compartments" again, though it should have run out before "Sweep Objects" -- are they tied to the same slice? Oh, it does look like queueForegroundObjectsForSweep goes without a pause.
 - Ouch. It marked for 37ms out of a budget of 40ms, then spent 17ms sweeping.

Comment 1

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

Attachment: Avoid budget overruns by not proceeding to sweep after mark

My first idea was to only continue from mark to sweep if you've used up less than 10 or 20% of the slice budget, but that would have required too many changes in SliceBudget, so I figured we can just eat the extra slice. Especially if it reduces our max pause.

On my machine, with the specific log I happen to be looking at, it would be way way better to just never combine them.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8788634 [details] [diff] [review]
Avoid budget overruns by not proceeding to sweep after mark

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

This is supposed to happen already but has been broken by bug 1245767.  The logic for doing this is the if block above starting |if (!lastMarkSlice ...|.

Note that the part that takes a long time is beginSweepingZoneGroup which is called from beginSweepPhase and happens before the extra yield added by this patch.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1300893-yield-before-sweep

The problem was adding the |useZeal| here and hoisting it out of the negation:

http://searchfox.org/mozilla-central/diff/f4ed7b8f012f256f6bf593c2e954c6a52c4d1ae7/js/src/jsgc.cpp#6122

The condition is complicated so I added some comments to hopefully explain this better.

Comment 4

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

Comment on attachment 8788826 [details] [diff] [review]
bug1300893-yield-before-sweep

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

Ah, thanks, I didn't read closely enough.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d660e7c7d2ef
Fix yielding before we start sweeping in multi-slice incremental GCs r=sfink

Comment 6

[Carsten Book [:Tomcat]]

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