Wait for cancelled Ion compilations in parallel rather than serially

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments)

Following on from bug 1304081, we can make CancelOffThreadIonCompile cancel all matching compilations before waiting for them so that we don't serialise on waiting for each of them to finish.
The patch refactors CancelOffThreadIonCompile to use a variant to select which compilations to cancel.

In addition it makes waiting for builders to cancel happen in parallel.

I removed FinishAllOffThreadCompilations since we determined it doesn't do anything that CancelOffThreadIonCompile doesn't already do and is only ever called directly after it.
Attachment #8793391 - Flags: review?(jdemooij)
This patch adds the ability to select compilations to cancel by runtime and by zone GC state, so we don't have to call this function in a loop any more.

I moved the call to CancelOffThreadIonCompile out of JitCompartment::sweep and into the GC itself and added an assert that there were no active compilations here.

jit::InvalidateAll is only called from Zone::discardJitCode.  I lifted the cancellation out of this and added an assert there too.

StopAllOffThreadCompilations didn't do anything more that call CancelOffThreadIonCompile at this point so I removed it.
Attachment #8793398 - Flags: review?(jdemooij)
Comment on attachment 8793398 [details] [diff] [review]
bug1304425-improve-cancel-compilation

Requesting review for the GC changes.
Attachment #8793398 - Flags: review?(terrence.d.cole)
Attachment #8793398 - Attachment is patch: true
Comment on attachment 8793398 [details] [diff] [review]
bug1304425-improve-cancel-compilation

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

\o/ This is excellent!
Attachment #8793398 - Flags: review?(terrence.d.cole) → review+
Blocks: gdoc_pageend
Comment on attachment 8793391 [details] [diff] [review]
bug1304425-refactor-cancel-compilation

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

Beautiful refactoring, thanks.

::: js/src/vm/HelperThreads.cpp
@@ +130,5 @@
> +    struct Matcher
> +    {
> +        JSRuntime* match(JSScript* script)    { return script->runtimeFromMainThread(); }
> +        JSRuntime* match(JSCompartment* comp) { return comp->runtimeFromMainThread(); }
> +        JSRuntime* match(AllCompilations all) { return nullptr; }

Maybe MOZ_CRASH? It doesn't really matter I guess.

::: js/src/vm/HelperThreads.h
@@ +421,5 @@
>  void
> +CancelOffThreadIonCompile(CompilationSelector selector, bool discardLazyLinkList);
> +
> +inline void
> +CancelOffThreadIonCompile(JSScript* script) {

Nit: { on its own line, also below
Attachment #8793391 - Flags: review?(jdemooij) → review+
Comment on attachment 8793398 [details] [diff] [review]
bug1304425-improve-cancel-compilation

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

I agree with Terrence.

::: js/src/jit/Ion.cpp
@@ +3128,3 @@
>  jit::InvalidateAll(FreeOp* fop, Zone* zone)
>  {
> +    // The caller should previously have cancelled off thred compilation.

Nit: thread

::: js/src/vm/HelperThreads.h
@@ +438,5 @@
>      CancelOffThreadIonCompile(CompilationSelector(comp), true);
>  }
>  
>  inline void
> +CancelOffThreadIonCompile(JSRuntime* runtime, JS::Zone::GCState state) {

Nit: { on next line
Attachment #8793398 - Flags: review?(jdemooij) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc84cb9eaeb7
Refactor CancelOffThreadIonCompile and make it wait for builders to cancel in parallel r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/50ffa88306ec
Cancel off thread compilations by runtime or zone GC state where possible r=jandem r=terrence
https://hg.mozilla.org/mozilla-central/rev/dc84cb9eaeb7
https://hg.mozilla.org/mozilla-central/rev/50ffa88306ec
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1305220
You need to log in before you can comment on or make changes to this bug.