Closed
Bug 862892
Opened 10 years ago
Closed 10 years ago
Support off-main-thread compilation for parallel execution
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: nmatsakis, Assigned: nmatsakis)
References
Details
(Whiteboard: [PJS])
Attachments
(1 file, 12 obsolete files)
128.16 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Assignee: general → nmatsakis
Assignee | ||
Comment 1•10 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•10 years ago
|
||
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•10 years ago
|
||
Attachment #750544 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 4•10 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•10 years ago
|
Whiteboard: PJS → [PJS]
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #750545 -
Flags: review?(shu)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #750554 -
Flags: review?(shu)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #750556 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #750556 -
Flags: review?(bhackett1024) → review+
Comment 10•10 years ago
|
||
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•10 years ago
|
||
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•10 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•10 years ago
|
||
Remove duplicate hunks and other mess from patch.
Attachment #750374 -
Attachment is obsolete: true
Attachment #751178 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 14•10 years ago
|
||
Cleaned up the patch.
Attachment #750545 -
Attachment is obsolete: true
Attachment #750545 -
Flags: review?(shu)
Attachment #751264 -
Flags: review?(shu)
Assignee | ||
Comment 15•10 years ago
|
||
Revised patch.
Attachment #750554 -
Attachment is obsolete: true
Attachment #750554 -
Flags: review?(shu)
Attachment #751266 -
Flags: review?(shu)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #750558 -
Attachment is obsolete: true
Attachment #750558 -
Flags: review?(shu)
Attachment #751268 -
Flags: review?(shu)
Comment 17•10 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•10 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•10 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•10 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•10 years ago
|
||
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•10 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•10 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 24•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #751178 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 25•10 years ago
|
||
Try run:
Assignee | ||
Comment 26•10 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=69940c6e9331
Assignee | ||
Comment 27•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bdb824158c6
Assignee | ||
Comment 28•10 years ago
|
||
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+
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2bdb824158c6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 30•10 years ago
|
||
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•10 years ago
|
||
Yes, that was their purpose, though actually I did not intend to commit those changes. I apologize!
You need to log in
before you can comment on or make changes to this bug.
Description
•