Closed Bug 1099463 Opened 5 years ago Closed 5 years ago

GC_SHRINK seems to clean up too much


(Core :: JavaScript: GC, defect)

Not set





(Reporter: sfink, Assigned: jonco)




(1 file)

I'm getting a strange assertion with my patches + cgc:

TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/gc/bug-975959.js | Assertion failure: activeParallelEntryScripts_ && activeParallelEntryScripts_->has(script), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jit/Ion.cpp:446 (code -11, args "--ion-eager --ion-offthread-compile=off")

What's happening is that notifyOfActiveParallelEntryScript with a script that gets added to the parallelEntryScripts_ table. Then we do a GC with gc.cleanUpEverything set to true. Then notifyOfActiveParallelEntryScript gets called again with that script, and we assert because it's an entry script but it isn't in the table.

gc.cleanUpEverything is set to true because GCRuntime::collect gets called with reason=DEBUG_GC, gckind=GC_SHRINK.

This seems to be intermittent on tbpl, but I can reproduce it 100% locally with a debug build.

The other reasons for setting cleanUpEverything seem pretty drastic (DESTROY_RUNTIME, SHUTDOWN_CC) and would happen when no scripts are running. But GC_SHRINK happens all the time with JS_GC_ZEAL=14, even with scripts active. So it looks to me like cleanUpEverything is too big of a hammer to use for GC_SHRINK. But I don't know this stuff well enough to know what should be cleaned up for a GC_SHRINK.
The patches are the ones for bug 1088831, btw, though I've had to fix up one of them a bit:

-    MOZ_ASSERT_IF(gcDepth == 1, phases[phase].parent == parent);
+    // Major and minor GCs can nest inside PHASE_GC_BEGIN/PHASE_GC_END.
+    MOZ_ASSERT_IF(gcDepth == 1 && phase != PHASE_MINOR_GC, phases[phase].parent == parent);

but I *think* my patches only coincidentally trigger this (I still don't know why.)
Flags: needinfo?(jcoppeard)
Blocks: 1088831
Reproduced.  I think it's reasonable to have cleanUpEverything set for a shrinking GC as a shrinking GC is supposed to reclaim as much memory as possible.

I think the problem is that we remove scripts from this table without clearing the flag that indicates that it's in the table.
Assignee: nobody → jcoppeard
Blocks: 650161
Flags: needinfo?(jcoppeard)
Attached patch bug-1099463Splinter Review
Patch to clear the parallel entry script flag when removing a script from the active parallel entry scripts table due to shrinking GC.
Attachment #8523809 - Flags: review?(shu)
Comment on attachment 8523809 [details] [diff] [review]

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

Thanks for fixing.
Attachment #8523809 - Flags: review?(shu) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.