862922 - Provide feedback on PJS bailout

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

We should indicate to users when parallel execution fails and why.  There are numerous levels of feedback we can provide, starting with simple warnings in the console and ranging up to full-blown GUIs.  This bug is intended to serve as a starting point, so the goal is to (1) expose the relevant information internally and then (2) offer a simple warning.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Improve bailouts to provide more feedback

The basic idea of this patch is to :

(1) change parallel aborts so that instead of having every parallel abort branch to a central location, we branch to unique locations depending on where and why the error occured;

(2) add a BailoutRecord that is kept on a per-thread basis;

(3) when bailing out, store the PC and other information into BailoutRecord. We potentially collect stack trace information as the parallel frames return (note that parallel bailouts are done via a series of returns).

(4) report a warning with the top-most script and line number giving the approximate cause of the bailout.

There are definitely improvements to be made here, but this is a first step.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 743386 [details] [diff] [review]
Improve bailouts to provide more feedback

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

::: js/src/ion/CodeGenerator.cpp
@@ +6054,3 @@
>  
> +    masm.move32(Imm32(cause), CallTempReg0);
> +    masm.movePtr(ImmWord((void *) script), CallTempReg1);

JSScript is a GC thing, so s/ImmWord/ImmGCPtr (and no cast).

@@ +6093,5 @@
> +CodeGenerator::visitOutOfLinePropagateParallelAbort(OutOfLinePropagateParallelAbort *ool)
> +{
> +    JSScript *script = ool->lir()->mirRaw()->block()->info().script();
> +
> +    masm.movePtr(ImmWord((void *) script), CallTempReg0);

And here.

::: js/src/ion/Lowering.cpp
@@ +118,4 @@
>      LParCheckOverRecursed *lir = new LParCheckOverRecursed(
>          useRegister(ins->parSlice()),
>          temp());
> +    lir->setMir(ins);

The add call below will do this if you use add(lir, ins)

@@ +1607,4 @@
>  LIRGenerator::visitParSlice(MParSlice *ins)
>  {
>      LParSlice *lir = new LParSlice(tempFixed(CallTempReg0));
> +    lir->setMir(ins);

You can rm this line, it's the first thing defineReturn does.

@@ +1616,5 @@
>  {
> +    LParWriteGuard *lir = new LParWriteGuard(useFixed(ins->parSlice(), CallTempReg0),
> +                                             useFixed(ins->object(), CallTempReg1),
> +                                             tempFixed(CallTempReg2));
> +    lir->setMir(ins);

Use add(lir, ins) instead.

@@ +1627,4 @@
>      LParCheckInterrupt *lir = new LParCheckInterrupt(
>          useRegister(ins->parSlice()),
>          temp());
> +    lir->setMir(ins);

And here.

::: js/src/ion/ParallelFunctions.cpp
@@ +135,3 @@
>      } else {
> +        realStackLimit = slice->perThreadData->ionStackLimit;
> +    }

Nit: no { }

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +283,5 @@
>      CompileInfo &info = snapshot->mir()->block()->info();
>      switch (info.executionMode()) {
>        case ParallelExecution: {
> +        // in parallel mode, make no attempt to recover, just signal an error.
> +        OutOfLineParallelAbort *ool = oolParallelAbort(ParallelBailoutUnsupported,

Is ParallelArray x86/x64-only? If not you should probably also update CodeGeneratorARM.

::: js/src/vm/ForkJoin.cpp
@@ +520,5 @@
> +static const char *
> +BailoutExplanation(ParallelBailoutCause cause)
> +{
> +    switch (cause) {
> +    case ParallelBailoutNone:

Nit: indent case labels with two spaces.

::: js/src/vm/ForkJoin.h
@@ +137,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

Passing in a...?

@@ +259,5 @@
> +    ParallelBailoutUnsupportedSparseArray,
> +};
> +
> +struct ParallelBailoutTrace {
> +    JSScript* script;

Nit: JSScript *script;

@@ +265,5 @@
> +};
> +
> +// See "Bailouts" section in comment above.
> +struct ParallelBailoutRecord {
> +    JSScript* topScript;

Ditto.

@@ +270,5 @@
> +    ParallelBailoutCause cause;
> +
> +    // Eventually we will support deeper traces,
> +    // but for now we gather at most a single frame.
> +    static const uint32_t maxDepth = 1;

Nit: s/maxDepth/MaxDepth

Comment 3

[Niko Matsakis [:nmatsakis]]

Clean try run: https://tbpl.mozilla.org/?tree=Try&rev=c5203da7df38 (SM only)

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Rebase, incorporate suggestions, fix non-threadsafe builds, and port fix for bug 855807

Carrying over r+ from jandem

Comment 5

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a44a72db55b

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a44a72db55b