862892 - Support off-main-thread compilation for parallel execution

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

The current parallel execution code blocks the main thread while it compiles the transitive closure of the kernel function and its callees.  This creates a noticeable delay.  A better strategy is to perform those compilations off the main thread and run the loop sequentially while waiting.

Comment 1

[Niko Matsakis [:nmatsakis]]

In bug 872191, nbp pointed out that the construction of the callee graph in ParallelArrayVisitor needs to be moved to the link phase.

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Remove transitive compilation logic from ion

This patch removes the transitive compilation logic from ion and generally tries to make parallel and sequential compilation go through the same code. Hence the "SequentialCompileContext" and "ParallelCompilexContext" are removed. Instead, there is a new parallel entry point ("CanEnterInParallel") that compiles a single script. We also enable off-main-thread compilation, which requires generalizing a few pathways that were specific to sequential compilation.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Move call target aggregation into link phase as it may invoke GC

Comment 4

[Niko Matsakis [:nmatsakis]]

Comment on attachment 750374 [details] [diff] [review]
Remove transitive compilation logic from ion

Changing review to bhackett as it seems more topical.

Comment 5

[Niko Matsakis [:nmatsakis]]

Attachment: Update ForkJoin to implement transitive compilation logic

Comment 6

[Niko Matsakis [:nmatsakis]]

Attachment: Changes to parallelarray.js to use new protocol

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: Misc. useful debug spew etc

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Update test infrastructure

Tests now try to stabilize after some number of compile bailouts and then guarantee at least one bailout-free, all-parallel execution.

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 750374 [details] [diff] [review]
Remove transitive compilation logic from ion

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

This patch contains a lot of duplicate sections.  Can you clean it up and reattach?

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 750544 [details] [diff] [review]
Move call target aggregation into link phase as it may invoke GC

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

This patch also has a lot of duplicate sections.

Comment 11

[Niko Matsakis [:nmatsakis]]

Attachment: Move call target aggregation into link phase as it may invoke GC

Sorry, was copying hunks around to separate the change into cleaner patches, must have made some duplicates.

Comment 12

[Niko Matsakis [:nmatsakis]]

Comment on attachment 750544 [details] [diff] [review]
Move call target aggregation into link phase as it may invoke GC

Forgot to mark old version as obsolete.

Comment 13

[Niko Matsakis [:nmatsakis]]

Attachment: Remove transitive compilation logic from ion

Remove duplicate hunks and other mess from patch.

Comment 14

[Niko Matsakis [:nmatsakis]]

Attachment: Update ForkJoin to implement transitive compilation logic

Cleaned up the patch.

Comment 15

[Niko Matsakis [:nmatsakis]]

Attachment: Changes to parallelarray.js to use new protocol

Revised patch.

Comment 16

[Niko Matsakis [:nmatsakis]]

Attachment: Update test infrastructure

Comment 17

[Shu-yu Guo [:shu]]

Comment on attachment 751264 [details] [diff] [review]
Update ForkJoin to implement transitive compilation logic

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

::: js/src/vm/ForkJoin.cpp
@@ +199,5 @@
> +    ModeBailout,
> +
> +    ModeMax
> +};
> +

Since this is in the scope of namespace js, I would give these longer prefixes than Mode.

Nit: Would prefer NumForkJoinModes over ModeMax.

@@ +224,5 @@
> +    // compilation follow a similar control-flow. They return RedLight
> +    // if they have either encountered a fatal error or completed the
> +    // execution, such that no further work is needed. In that event,
> +    // they take an `ExecutionStatus*` which they use to report
> +    // whether execution was successful or what. If the function

Nit: or what -> or not

@@ +245,5 @@
> +    TrafficLight warmupExecution(ExecutionStatus *status);
> +    TrafficLight parallelExecution(ExecutionStatus *status);
> +    TrafficLight sequentialExecution(bool disqualified, ExecutionStatus *status);
> +    TrafficLight recoverFromBailout(ExecutionStatus *status);
> +    TrafficLight fatalError(ExecutionStatus *status);

Nice, the state machine makes transitive compilation pretty easy to reason about.

@@ +709,1 @@
>  

Don't we need another loop here in case some scripts got invalidated due to type propagation during transitive compilation?

@@ +985,5 @@
> +
> +        const types::CompilerOutput &cout = *co.compilerOutput(
> +            cx_->compartment->types); // XXX
> +        Spew(SpewBailouts, "co.script=%p, co.kind=%d, isValid=%d", // XXX
> +             cout.script, cout.kind(), cout.isValid()); // XXX

What's up with these XXXs?

@@ +1114,3 @@
>  
> +    // after any bailout, we always scan over callee list of main
> +    // function, if nothing else

Nit: capitalize

@@ +1118,5 @@
> +    if (!addToWorklist(mainScript))
> +        return fatalError(status);
> +
> +    // also invalidate and recompile any callees that were implicated
> +    // by the bailout

Ditto

::: js/src/vm/ForkJoin.h
@@ +160,5 @@
> +// Bailout tracing and recording:
> +//
> +// When a bailout occurs, we have to record a bit of state so that we
> +// can recover with grace.  The caller of ForkJoin is responsible for
> +// passing in a.  This state falls into two categories: one is

Missing a word here.

@@ +172,5 @@
> +//   will be invalidated.  As part of ParallelDo, the top-most script
> +//   from each stack frame will be invalidated.
> +//
> +// - Second, for each script on the stack, we will set the flag
> +//   HasInvalidatedCallTarget, indicating that some callee of this

-> hasUncompiled

@@ +178,5 @@
> +//   during the bailout.
> +//
> +// The optional state consists of a backtrace of (script, bytecode)
> +// pairs.  The rooting on these is currently screwed up and needs to
> +// be fixed.

Add a FIXME if this is the case.

Comment 18

[Shu-yu Guo [:shu]]

Comment on attachment 751266 [details] [diff] [review]
Changes to parallelarray.js to use new protocol

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

::: js/src/builtin/ParallelArray.js
@@ +378,5 @@
>  
>    var self = this;
>    var length = self.shape[0];
>    var buffer = NewDenseArray(length);
> +  var blockForCompilation = mode && mode.blockForCompilation;

I don't see any uses of this.

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 751268 [details] [diff] [review]
Update test infrastructure

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

::: js/src/jit-test/lib/parallelarray-helpers.js
@@ +170,5 @@
> +// where the `new ParallelArray(...)` is a stand-in
> +// for some parallel array operation.
> +function assertParallelExecWillBail(opFunction) {
> +  opFunction({mode:"compile"}); // get the script compiled
> +  opFunction({mode:"bailout"}); // check that it bails when executed

Is this way of testing going to play nice with gczeal? Are we going to get random failures because some test harness might set the GCZEAL env var too high and cause the jitcode to be swept between these calls?

Comment 20

[Niko Matsakis [:nmatsakis]]

Shu---

Nice catch regarding the need for an extra loop to catch GC invalidations, you're absolutely correct.

As far as GCZeal goes, it's certainly possible that this could cause problems. Probably the easiest thing to do is just disable the checking that par exec succeeds when running with GCZeal.

Comment 21

[Niko Matsakis [:nmatsakis]]

Attachment: Update ForkJoin/ParallelArray.js to implement transitive compilation logic

Nits addressed, also added fixes regarding zeal and double checking that scripts have been compiled. Since ForkJoin code did not get r+ before, resubmitting for r?.

Comment 22

[Shu-yu Guo [:shu]]

Comment on attachment 751522 [details] [diff] [review]
Update ForkJoin/ParallelArray.js to implement transitive compilation logic

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

::: js/src/vm/ForkJoin.cpp
@@ +728,5 @@
> +        // to begin again.
> +        bool allScriptsPresent = true;
> +        for (uint32_t i = 0; i < worklist_.length(); i++) {
> +            if (!worklist_[i]->hasParallelIonScript()) {
> +                calleesEnqueued_[i] = false;

Hm, wouldn't this re-enqueue multiple copies of some previously invalidated script's callees?

Comment 23

[Niko Matsakis [:nmatsakis]]

> Hm, wouldn't this re-enqueue multiple copies of some previously invalidated script's callees?

No, it will never add the same script to the worklist twice. That flag just controls whether we *check* the callee list.

Comment 24

[Brian Hackett [Laid off!]]

Comment on attachment 751170 [details] [diff] [review]
Move call target aggregation into link phase as it may invoke GC

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

::: js/src/ion/ParallelArrayAnalysis.h
@@ +56,5 @@
> +// Code to collect list of possible call targets by scraping through
> +// TI and baseline data. Used to permit speculative transitive
> +// compilation in vm/ForkJoin.
> +//
> +// WARNING: This code may clone scripts and thus may invoke the GC.

rm the 'WARNING'.

Comment 25

[Niko Matsakis [:nmatsakis]]

Try run:

Comment 26

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=69940c6e9331

Comment 27

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bdb824158c6

Comment 28

[Niko Matsakis [:nmatsakis]]

Attachment: Final version of the patch.

Includes a few minor bugfixes encountered during try runs.

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2bdb824158c6

Comment 30

[Douglas Crosher [:dougc]]

The changes to 'spewPass' prevent output of the 'spew' when
compiling asm.js code as it does not set a JSContext when
initializing the IonContext.  Could these changes have been
a guard to prevent crashes in the spew output?  If so then
perhaps bug 871242 could address them.

Comment 31

[Niko Matsakis [:nmatsakis]]

Yes, that was their purpose, though actually I did not intend to commit those changes. I apologize!