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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: 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.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: better patch

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1273cc549e2e

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1273cc549e2e

Comment 5

[Brian Hackett [Laid off!]]

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.

Comment 6

[Brian Hackett [Laid off!]]

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

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

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1e3e9febdc2e