Support off-main-thread compilation for parallel execution

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nmatsakis, Assigned: nmatsakis)

Tracking

22 Branch
mozilla24
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PJS])

Attachments

(1 attachment, 12 obsolete attachments)

128.16 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: general → nmatsakis
(Assignee)

Updated

5 years ago
Blocks: 801869
Whiteboard: PJS
(Assignee)

Updated

5 years ago
Blocks: 856246
(Assignee)

Updated

5 years ago
Blocks: 865931
(Assignee)

Comment 1

5 years ago
In bug 872191, nbp pointed out that the construction of the callee graph in ParallelArrayVisitor needs to be moved to the link phase.
(Assignee)

Comment 2

5 years ago
Created attachment 750374 [details] [diff] [review]
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.
Attachment #750374 - Flags: review?(dvander)
(Assignee)

Comment 3

5 years ago
Created attachment 750544 [details] [diff] [review]
Move call target aggregation into link phase as it may invoke GC
Attachment #750544 - Flags: review?(bhackett1024)
(Assignee)

Comment 4

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

Changing review to bhackett as it seems more topical.
Attachment #750374 - Flags: review?(dvander) → review?(bhackett1024)
(Assignee)

Updated

5 years ago
Whiteboard: PJS → [PJS]
(Assignee)

Comment 5

5 years ago
Created attachment 750545 [details] [diff] [review]
Update ForkJoin to implement transitive compilation logic
Attachment #750545 - Flags: review?(shu)
(Assignee)

Comment 6

5 years ago
Created attachment 750554 [details] [diff] [review]
Changes to parallelarray.js to use new protocol
Attachment #750554 - Flags: review?(shu)
(Assignee)

Comment 7

5 years ago
Created attachment 750556 [details] [diff] [review]
Misc. useful debug spew etc
Attachment #750556 - Flags: review?(bhackett1024)
(Assignee)

Comment 8

5 years ago
Created attachment 750558 [details] [diff] [review]
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.
Attachment #750558 - Flags: review?(shu)
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?
Attachment #750374 - Flags: review?(bhackett1024)
Attachment #750556 - Flags: review?(bhackett1024) → review+
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.
Attachment #750544 - Flags: review?(bhackett1024)
(Assignee)

Comment 11

5 years ago
Created attachment 751170 [details] [diff] [review]
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.
Attachment #751170 - Flags: review?(bhackett1024)
(Assignee)

Comment 12

5 years ago
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.
Attachment #750544 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Created attachment 751178 [details] [diff] [review]
Remove transitive compilation logic from ion

Remove duplicate hunks and other mess from patch.
Attachment #750374 - Attachment is obsolete: true
Attachment #751178 - Flags: review?(bhackett1024)
(Assignee)

Comment 14

5 years ago
Created attachment 751264 [details] [diff] [review]
Update ForkJoin to implement transitive compilation logic

Cleaned up the patch.
Attachment #750545 - Attachment is obsolete: true
Attachment #750545 - Flags: review?(shu)
Attachment #751264 - Flags: review?(shu)
(Assignee)

Comment 15

5 years ago
Created attachment 751266 [details] [diff] [review]
Changes to parallelarray.js to use new protocol

Revised patch.
Attachment #750554 - Attachment is obsolete: true
Attachment #750554 - Flags: review?(shu)
Attachment #751266 - Flags: review?(shu)
(Assignee)

Comment 16

5 years ago
Created attachment 751268 [details] [diff] [review]
Update test infrastructure
Attachment #750558 - Attachment is obsolete: true
Attachment #750558 - Flags: review?(shu)
Attachment #751268 - Flags: review?(shu)

Comment 17

5 years ago
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.
Attachment #751264 - Flags: review?(shu)

Comment 18

5 years ago
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.
Attachment #751266 - Flags: review?(shu) → review+

Comment 19

5 years ago
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?
Attachment #751268 - Flags: review?(shu) → review+
(Assignee)

Comment 20

5 years ago
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.
(Assignee)

Comment 21

5 years ago
Created attachment 751522 [details] [diff] [review]
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?.
Attachment #751264 - Attachment is obsolete: true
Attachment #751266 - Attachment is obsolete: true
Attachment #751522 - Flags: review?(shu)

Comment 22

5 years ago
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?
Attachment #751522 - Flags: review?(shu) → review+
(Assignee)

Comment 23

5 years ago
> 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 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'.
Attachment #751170 - Flags: review?(bhackett1024) → review+
Attachment #751178 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 25

5 years ago
Try run:
(Assignee)

Comment 26

5 years ago
Try run: https://tbpl.mozilla.org/?tree=Try&rev=69940c6e9331
(Assignee)

Comment 27

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bdb824158c6
(Assignee)

Comment 28

5 years ago
Created attachment 754121 [details] [diff] [review]
Final version of the patch.

Includes a few minor bugfixes encountered during try runs.
Attachment #750556 - Attachment is obsolete: true
Attachment #751170 - Attachment is obsolete: true
Attachment #751178 - Attachment is obsolete: true
Attachment #751268 - Attachment is obsolete: true
Attachment #751522 - Attachment is obsolete: true
Attachment #754121 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2bdb824158c6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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.
(Assignee)

Comment 31

5 years ago
Yes, that was their purpose, though actually I did not intend to commit those changes. I apologize!
Depends on: 886101
You need to log in before you can comment on or make changes to this bug.