1555199 - looping based on isIncrementalGCInProgress is unreliable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

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

I ran into this as a result of my "Set gcLastSweepGroupIndex earlier" patch in bug 1167452.

Anyway, consider https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/js/src/jsapi-tests/testWeakMap.cpp#100-102

I am getting a case where the GC is mostly done, in the Finalize state. When debugGCSlice runs, it finishes that GC off and starts a new one. Then when it compares sweep group indexes, the old code never cleared the indexes and hasn't yet set new ones, so it works. The new code clears them out when starting a new GC.

So the question is, should this be using finishGC if it wants to finish the GC? In that case, the new code is fine; it won't restart the GC so it still has access to the previous GC's indexes.

Or do we want to say that the loop should be a valid way to finish up a GC (without starting a new one)? I haven't worked out what the minimal change would be for that.

Comment 1

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

Attachment: Bug 1555199 - Use finishGC to finish a GC instead of looping r=jonco

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #0)

When debugGCSlice runs, it finishes that GC off and starts a new one.

Why does this happen? I would have thought it just finished the current one.

So the question is, should this be using finishGC if it wants to finish the GC?

Yes, that would be clearer than looping so I think we should do that.

Comment 3

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

(In reply to Jon Coppeard (:jonco) from comment #2)

(In reply to Steve Fink [:sfink] [:s:] from comment #0)

When debugGCSlice runs, it finishes that GC off and starts a new one.

Why does this happen? I would have thought it just finished the current one.

I think I fooled myself into misinterpreting this.

I was looking at the GC number to determine which GC I was looking at. But that is also incremented for minor GCs; I should have been looking at the major GC number. (Well, I was also basing it off of when the zone's sweep group index was being reset, but that was buggy -- see below.)

Though in stepping through this, one thing seems a little problematic -- while we are in the part of an incremental GC where we wait for first background finalization and then decommitting, we're constantly doing nursery collections and thus expensively tenuring everything. shouldCollectNurseryForSlice explicitly returns true for Finalize and Decommit -- why is that? (needinfo pbone for that.)

The real bug is in how I was clearing the zone's sweep group index. I hadn't realized that zones get constantly scheduled and unscheduled during an incremental GC. I was clearing it during scheduling. So while the patch here is probably fine to land, it won't fix what I was working on.

Comment 4

[Paul Bone [:pbone]]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)

Though in stepping through this, one thing seems a little problematic -- while we are in the part of an incremental GC where we wait for first background finalization and then decommitting, we're constantly doing nursery collections and thus expensively tenuring everything. shouldCollectNurseryForSlice explicitly returns true for Finalize and Decommit -- why is that? (needinfo pbone for that.)

Because I didn't understand them at the time I wrote that code and was playing it safe. We can fix that (Bug 1555558).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8b0efeaaa35
Use finishGC to finish a GC instead of looping r=jonco

Comment 6

[Narcis Beleuzu [:NarcisB]]

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