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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jandem, Assigned: djvj)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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

6 years ago
We should report this upstream -- no reason they should be writing to out-of-bounds elements.
(Assignee)

Updated

5 years ago
Assignee: general → kvijayan
(Assignee)

Comment 2

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

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

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

Comment 7

5 years ago
Pushed Baseline IC add OOB TypedArray SetElem stub:
https://hg.mozilla.org/projects/ionmonkey/rev/8565e1fcdf91
(Assignee)

Comment 8

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

Comment 9

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

Comment 10

5 years ago
Created attachment 721865 [details] [diff] [review]
Handle OOB TypedArray SetElem writes in Ion. (try 2)

Second round, this time with MTypedArraySetElementHole.
Attachment #721291 - Attachment is obsolete: true
Attachment #721865 - Flags: review?(jdemooij)
(Reporter)

Comment 11

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

Comment 12

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

Comment 13

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

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [leave open]

Comment 14

5 years ago
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

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

Comment 16

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.