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

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Other Branch
mozilla20
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 687813 [details] [diff] [review]
patch

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

Comment 1

6 years ago
Created attachment 687829 [details] [diff] [review]
better patch
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Comment 5

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

Comment 6

6 years ago
Revert to original patch (r=billm over IRC):

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3e9febdc2e

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/1e3e9febdc2e
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.