As discussed on IRC, the control flow here is monumentally complicated, but cannot actually trigger GC in practice. Unfortunately, because of the field call when we dispatch to the sub-threads, the analysis recognize will not recognize that the AutoAssertNoGC covers the parallel section. Instead we have to make the assertion in parallel. This means that getting the runtime out of TLS will not work, so I have added a JSRuntime* variant of AutoAssertNoGC. When we have a JSRuntime, it is generally safe to access it, except through the pointer in ForkJoinShared: i.e. the one we are using here. Fortunately, we know for other reasons that this usage is safe since the main thread is blocked or doing other things that cannot GC. At the very least, we want to over-assert here rather than under asserting, so I think the new constructor is preferable to ignoring the assertions in this case. Flagging Steve for review of the AutoAssertNoGC changes and Shu for the changes to ForkJoin.

Comment on attachment 828379 [details] [diff] [review] hazard_forkjoin-v0.diff Review of attachment 828379 [details] [diff] [review]: ----------------------------------------------------------------- The patch doesn't contain any AutoAssertNoGC changes. r-, it'll never work, rejecting with prejudice. ;-) (I'm guessing you're missing a qref.)

Yes indeed, I forgot to qref. Luckily, my changes lined up with just the right split, so I qnewd instead of qrefing.

Comment on attachment 828676 [details] [diff] [review] hazard_forkjoin_autoassert_changes-v0.diff Review of attachment 828676 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsgc.cpp @@ +5306,5 @@ > } > > JS::AutoAssertNoGC::~AutoAssertNoGC() > { > + MOZ_ASSERT(gcNumber == runtime->gcNumber); You no like my MOZ_ASSERT explanation text?

(In reply to Terrence Cole [:terrence] from comment #2) > Created attachment 828676 [details] [diff] [review] > hazard_forkjoin_autoassert_changes-v0.diff > > Yes indeed, I forgot to qref. Luckily, my changes lined up with just the > right split, so I qnewd instead of qrefing. Well, mostly. There's a chunk in ForkJoinSlice.cpp in the 2nd patch, so you'll need to merge these before landing.

(In reply to Steve Fink [:sfink] from comment #3) > ::: js/src/jsgc.cpp > @@ +5306,5 @@ > > } > > > > JS::AutoAssertNoGC::~AutoAssertNoGC() > > { > > + MOZ_ASSERT(gcNumber == runtime->gcNumber); > > You no like my MOZ_ASSERT explanation text? Whoops! Did not mean to drop that. (In reply to Steve Fink [:sfink] from comment #4) > (In reply to Terrence Cole [:terrence] from comment #2) > > Created attachment 828676 [details] [diff] [review] > > hazard_forkjoin_autoassert_changes-v0.diff > > > > Yes indeed, I forgot to qref. Luckily, my changes lined up with just the > > right split, so I qnewd instead of qrefing. > > Well, mostly. There's a chunk in ForkJoinSlice.cpp in the 2nd patch, so > you'll need to merge these before landing. And yes, I appear to be totally incompetent after 5PM when I wake up at 7AM.

Lets just fold this all together now.

Comment on attachment 828849 [details] [diff] [review] hazard_forkjoin-v1.diff Review of attachment 828849 [details] [diff] [review]: ----------------------------------------------------------------- Sorry this took so long. Looks fine to me. I assume all PJS tests pass? ::: js/src/jsgc.cpp @@ +5312,1 @@ > } These changes look for PJS. What about Brian's off-thread stuff? Does requiring the runtime field to be always present break anything there?

These changes look fine for PJS*.

(In reply to Shu-yu Guo [:shu] from comment #7) > > Sorry this took so long. Looks fine to me. I assume all PJS tests pass? I ran jit-tests --tbpl with tests matching "arallel", which I think catches everything relevant. I'll do a full run before landing though. > These changes look for PJS. What about Brian's off-thread stuff? Does > requiring the runtime field to be always present break anything there? Good thought! I'll add an --ion-parallel-compile=on pass as well.

Looks good: https://hg.mozilla.org/integration/mozilla-inbound/rev/60122ebcfee3