Closed Bug 817635 Opened 8 years ago Closed 8 years ago

IonMonkey: don't compile off thread in compartments created during a global IGC

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
With bug 815906 landed, decoder is seeing assertions where we are still trying to mark things from the compilation thread.  What I think is happening is:

1. During a global incremental GC, a new compartment is created between IGC slices.  That compartment does not have needsBarrier() set.

2. Still before the next slice starts, we try to compile code in that compartment, and do it off thread because barriers are not set on the compartment.

3. Off thread compilation writes an atom somewhere (don't know where, don't have a full stack), which triggers a write barrier on the atoms compartment since it has needsBarrier() set.

I don't know the GC well enough to determine whether this is actually possible, i.e. whether compartments created during an IGC don't interrupt the GC or set needsBarrier() on the new compartment.  From cursory examination of the code this seems to be the case though, at least in the JS shell.

The attached patch is a spotfix to avoid compiling code in compartments that don't need barriers but can point to the atoms compartment, which does.  It would be nice though if we maintained an invariant that this wasn't actually possible, as it seems weird given that compartments can freely maintain pointers into the atoms compartment.
Attachment #687813 - Flags: review?(dvander)
Attached patch better patchSplinter Review
Assignee: general → bhackett1024
Attachment #687813 - Attachment is obsolete: true
Attachment #687813 - Flags: review?(dvander)
Attachment #687829 - Flags: review?(wmccloskey)
Comment on attachment 687829 [details] [diff] [review]
better patch

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

::: js/src/ion/Ion.cpp
@@ +1182,5 @@
>      // performed off thread during an incremental GC, as doing so may trip
>      // incremental read barriers.
>      if (js_IonOptions.parallelCompilation &&
>          OffThreadCompilationAvailable(cx) &&
> +        cx->runtime->gcIncrementalState == gc::NO_INCREMENTAL)

I just remembered that we have IsIncrementalGCInProgress(rt), which is slightly better than this. Please use that.
Attachment #687829 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/1273cc549e2e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
This fix didn't have the desired effect, as IsIncrementalGCInProgress returns false if the barrier verifier is running, which also enables barriers on compartments and can cause marking to occur off thread.  Using the original cx->runtime->gcIncrementalState == gc::NO_INCREMENTAL test instead should fix this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Revert to original patch (r=billm over IRC):

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3e9febdc2e
https://hg.mozilla.org/mozilla-central/rev/1e3e9febdc2e
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.