845873 - IonMonkey: Baseline: Fix Octane-gameboy out-of-bounds typed array writes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

The Octane-gameboy benchmark, in GameBoyCore.prototype.dispatchDraw, has the following loop:

  for (var canvasIndex = 0; canvasIndex < canvasRGBALength; ++canvasIndex) {
      canvasData[canvasIndex++] = frameBuffer[bufferIndex++];
      canvasData[canvasIndex++] = frameBuffer[bufferIndex++];
      canvasData[canvasIndex++] = frameBuffer[bufferIndex++];
  }

Problem is that canvasRGBALength is 92160 and canvasData.length is only 23040, so most of the writes are out-of-bounds... IonMonkey just hoists the bounds check and keeps bailing out, we don't currently have a way to detect out-of-bounds typed array SETELEM's.

Numbers with parallel compilation (inbound is JM+Ion, baseline is BL+Ion):

- inbound: 14111
- baseline: 12438

If I change the loop to iterate from 0 to canvasData.length:

- inbound: 15312
- baseline: 17341

So it looks like this is the main problem BL has with this benchmark, and fixing it can be a nice win on this benchmark.

What we should do is the following:

(1) Baseline: Add a bit to SetElem_TypedArray's key to indicate we've seen out-of-bounds writes. The IC stub can then just ignore these writes (see obj_setElement in jstypedarray.cpp).

(2) Have Ion query the baseline IC and add an MStoreTypedArrayHole instruction, similar to MStoreElementHole, so that it doesn't bail out.

Comment 1

[Jeff Walden [:Waldo]]

We should report this upstream -- no reason they should be writing to out-of-bounds elements.

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: Change TypedArray SetElem stubs to handle OOB writes

This patch just modifies Baseline's SetElem_TypedArray stubs to handle OOB writes.  Also, the fact that an out-of-bounds write was seen at the site is recorded in the stub.

Ion is not modified (yet), that's for a later patch.

Comment 3

[Kannan Vijayan [:djvj]]

I implemented the Ion side of this and found that there's not much improvement in performance (above what just the optimized stub provides).  We bail out a few times from the function you mentioned, and then bailoutExpected() keeps it from getting compiled again.

There are ~450 or so hits to the UseCount fallback stub for the loop, and it resets the useCount every time, so we end up missing about 450k opportunities to compile this routine.

Handling bailoutExpected properly will be somewhat to improving performance here.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 720841 [details] [diff] [review]
Change TypedArray SetElem stubs to handle OOB writes

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

Nice.

::: js/src/ion/BaselineIC.cpp
@@ +3987,5 @@
> +
> +    if (expectOutOfBounds_) {
> +        masm.bind(&expectOOBSuccess);
> +        // Allow any primitive for an OOB write, but not objects.
> +        masm.branchTestPrimitive(Assembler::NotEqual, value, &failure);

I think you can just remove this check. Looking at obj_setElement and toDoubleForTypedArray, objects are treated as 0 (int arrays) or NaN (for double arrays), we don't call valueOf or toString on them.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Kannan Vijayan [:djvj] from comment #3)
> I implemented the Ion side of this and found that there's not much
> improvement in performance (above what just the optimized stub provides). 
> We bail out a few times from the function you mentioned, and then
> bailoutExpected() keeps it from getting compiled again.

Do you know why we are bailing out? In general, bailing out without invalidating the IonScript can be a serious perf fault, especially with baseline.

Comment 6

[Kannan Vijayan [:djvj]]

Here is what happens:

dispatchDraw compiles for the first time before baseline has ever seen an OOB write on |canvasData|.  It compiles without the OOB-expecting SetElem, and than bails when the OOB write comes through.  This causes |failedBoundsCheck| to get set on the script, and the ioncode is invalidated.

It recompiles shortly after and runs, but bails out because somewhere within the call to |graphicsBlit| after the loop, there's some TI changes that cause the ioncode for |dipatchDraw| to get invalidated while still on stack.  When returning, it bails.

It's compiled a third time, and seems to run for a reasonable amount of time, until the ionCode gets GCed, returning it back to baseline.

The GC clears out baseline's IC info, so when baseline goes to re-compile it, it compiles Ion to bail on OOB.  However, during this bailout, the |failedBoundsCheck| flag is already set on the script, the ion code doesn't get invalidated.

Changing |HandleBoundsCheckFailure| to always invalidate the underlying script gets us the following nice scores (taken from doing 3 runs in a row and picking the median run):

OLD (with the optimized stub, but without Ion handling expected OOB)
===
Richards: 13083
DeltaBlue: 16410
Crypto: 19548
RayTrace: 27972
EarleyBoyer: 24046
RegExp: 3212
Splay: 16852
NavierStokes: 24881
PdfJS: 9899
Mandreel: 19348
Gameboy: 15637
CodeLoad: 13022
Box2D: 24875
----
Score (version 8): 15756


NEW (with optimized stub AND Ion handling expected OOB)
===
Richards: 13069
DeltaBlue: 15716
Crypto: 19326
RayTrace: 27944
EarleyBoyer: 24075
RegExp: 3295
Splay: 17097
NavierStokes: 25030
PdfJS: 12562
Mandreel: 18864
Gameboy: 18115
CodeLoad: 13048
Box2D: 25228
----
Score (version 8): 16207

The individual scores across runs are within their natural variations, but we see a consistent nice bump in both PdfJS and (from 10k to 12.5k) and Gameboy (15.5k to 18k).

I'll post the Ion patch today.

Comment 7

[Kannan Vijayan [:djvj]]

Pushed Baseline IC add OOB TypedArray SetElem stub:
https://hg.mozilla.org/projects/ionmonkey/rev/8565e1fcdf91

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: Handle OOB TypedArray SetElem writes in Ion.

There's a couple of miscellaneous changes here - one to BaselineBailouts Spew to report the OP that was bailed on.  Another to spew from the UseCount fallback when we fail-to-compile because of bailoutExpected.

Also, I'm starting to break out the BaselineInspector functionality into specific chain-specific inspectors that are created from BaselineInspector.  This seems to allow a much more readable naming of inspector methods - since they can assume the context of the chain-type-specific class they belong to, and it doesn't have to be encoded in the name.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 721291 [details] [diff] [review]
Handle OOB TypedArray SetElem writes in Ion.

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

::: js/src/ion/BaselineBailouts.cpp
@@ +618,5 @@
>      JS_ASSERT_IF(op != JSOP_FUNAPPLY || !iter.moreFrames() || resumeAfter,
>                   exprStackSlots == expectedDepth);
>  #endif
>  
> +    IonSpew(IonSpew_BaselineBailouts, "      Resuming %s pc offset %d (op %s) (line %d) of %s:%d",

Nit: while you are here, can you also add this somewhere:

#ifdef TRACK_SNAPSHOTS
    iter.spewBailingFrom();
#endif

It prints the name of the MIR/LIR instruction etc.

::: js/src/ion/BaselineInspector.h
@@ +30,5 @@
> +      : inspector_(inspector), pc_(pc), icEntry_(icEntry)
> +    { }
> +};
> +
> +class SetElemICInspector : public ICInspector

Nice, good idea.

::: js/src/ion/IonBuilder.cpp
@@ +5930,5 @@
> +            return false;
> +
> +        skipBlock = newBlock(current, pc);
> +        if (!skipBlock)
> +            return false;

I think it'd be better to do something similar to MStoreElementHole, MLoadTypedArrayElementHole etc: emit the bounds check as part of the op. It's more consistent and a bit less heavy-weight than having multiple blocks.

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: Handle OOB TypedArray SetElem writes in Ion. (try 2)

Second round, this time with MTypedArraySetElementHole.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 721865 [details] [diff] [review]
Handle OOB TypedArray SetElem writes in Ion. (try 2)

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

Nice. It would be good to add some tests with OOB writes to at least Uint8ClampedArray, Int32Array and Float32Array (I don't know if we have good jit-tests for this atm).

::: js/src/ion/MIR.h
@@ +4556,5 @@
> +        setOperand(0, elements);
> +        setOperand(1, length);
> +        setOperand(2, index);
> +        setOperand(3, value);
> +        setResultType(MIRType_Value);

You can remove this line (also from StoreTypedArrayElement), it does not return a value.

@@ +4560,5 @@
> +        setResultType(MIRType_Value);
> +        setMovable();
> +        JS_ASSERT(elements->type() == MIRType_Elements);
> +        JS_ASSERT(length->type() == MIRType_Int32);
> +        JS_ASSERT(index->type() == MIRType_Int32);

This assert is invalid (if the index is a phi it has type MIRType_Value until type analysis), please add a test that fails with the current patch.

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #11)
> This assert is invalid (if the index is a phi it has type MIRType_Value
> until type analysis), please add a test that fails with the current patch.

Sorry, scratch this, IonBuilder explicitly adds MToInt32.

Comment 13

[Kannan Vijayan [:djvj]]

Nits addressed and pushed:
https://hg.mozilla.org/projects/ionmonkey/rev/8436ace52390

However, this will reveal an existing bug in baseline that'll make tbpl go orange.
See Bug 848679.

Watching this until that gets addressed and tbpl goes green-ish again.

Comment 14

[Grant Galitz]

The OOB issue was due to the changes the octane team made to the gameboy-online project. The fake canvas they passed in is of the wrong size, so we OOB here. The array should be number of pixels times four, because it has separate values each for R, G, B, and A. The array the octane team passed seems to be sized to number of pixels and not number of pixels times color channels.

Comment 15

[Grant Galitz]

The variable canvasRGBALength was named so on purpose to alert someone about RGBA specificity.

Comment 16

[Grant Galitz]

Also when I finish debugging and fixing https://github.com/grantgalitz/IodineGBA maybe it should also be an eventual benchmark. It's a gameboy advance emulator whereas gbemu is a gameboy & gameboy color emulator. It would be interesting to bench an ARMv4T instruction set CPU emulation for js engines.