GC_SHRINK seems to clean up too much

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript: GC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sfink, Assigned: jonco)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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
(Assignee)

Comment 2

4 years ago
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)
(Assignee)

Comment 3

4 years ago
Created attachment 8523809 [details] [diff] [review]
bug-1099463

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 4

4 years ago
Comment on attachment 8523809 [details] [diff] [review]
bug-1099463

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

Thanks for fixing.
Attachment #8523809 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/0b76fb98e53c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.