Convert SETELEM dense/typed array IC to CacheIR

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: evilpie, Assigned: jandem)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 months ago
The dense/typed array SETELEM code is not yet ported to CacheIR and duplicated (differently) in Ion and Baseline.

We should make sure the dense element stubs work for all objects with dense elements and not just Arrays!
(Assignee)

Updated

4 months ago
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Updated

4 months ago
Depends on: 1337871
(Assignee)

Comment 1

4 months ago
Created attachment 8835472 [details] [diff] [review]
Part 1 - Setting dense/unboxed elements

Pretty straight-forward. The old code did too many things which makes it hard to understand. Splitting it up like this makes it much more readable.

That said, the stub for *adding* elements is the scary one, but one step at a time :)
Attachment #8835472 - Flags: review?(evilpies)
(Reporter)

Comment 2

4 months ago
Comment on attachment 8835472 [details] [diff] [review]
Part 1 - Setting dense/unboxed elements

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

Much better, but this was the simpler part.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1055,5 @@
> +    // We need to convert int32 values being stored into doubles. Note that
> +    // double arrays are only created by IonMonkey, so if we have no FP support
> +    // Ion is disabled and there should be no double arrays.
> +    if (cx_->runtime()->jitSupportsFloatingPoint)
> +        masm.convertInt32ValueToDouble(val);

From what I understand this is fine in Baseline, but changing the type like this can be tricky.

::: js/src/jit/BaselineInspector.h
@@ -36,5 @@
>      { }
>  
>      bool sawOOBDenseWrite() const;
>      bool sawOOBTypedArrayWrite() const;
> -    bool sawDenseWrite() const;

Wasn't even used ..

::: js/src/jit/CacheIR.cpp
@@ +2243,5 @@
> +    if (!obj->isNative())
> +        return false;
> +
> +    NativeObject* nobj = &obj->as<NativeObject>();
> +    if (!nobj->containsDenseElement(index) || nobj->getElementsHeader()->isFrozen())

Amazing how much simpler this when not handling this case after the SetElem call and not conflating the add/set case.
Attachment #8835472 - Flags: review?(evilpies) → review+
(Assignee)

Comment 3

4 months ago
(In reply to Tom Schuster [:evilpie] from comment #2)
> From what I understand this is fine in Baseline, but changing the type like
> this can be tricky.

Yes, I'll add a comment mentioning this is fine in Baseline but not for Ion.
(Assignee)

Comment 4

4 months ago
Created attachment 8836025 [details] [diff] [review]
Part 2 - Setting Dense/UnboxedArray holes

Removes a few hundred lines of code, and optimizes more things than before:

* We now handle setting holes at indexes < initLength.

* When we do JSOP_INITELEM_ARRAY etc we no longer guard on the prototype shapes as these objects are not relevant.
Attachment #8836025 - Flags: review?(evilpies)
(Assignee)

Comment 5

4 months ago
Oh and --unboxed-arrays doesn't even pass jit-tests. I verified the new code works with some simple tests but if I had to start over porting stubs to CacheIR I'd probably have rmeoved the unboxed array code :/

Comment 6

4 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96454c1e87ee
part 1 - Convert Baseline stubs for setting dense/unboxed elements to CacheIR. r=evilpie
(Assignee)

Updated

4 months ago
Keywords: leave-open
(Assignee)

Comment 7

4 months ago
Part 1 was an unexpected 5.5-8.5% win on sunspider string-tagcloud on AWFY.

The win comes from the SetElem here:

    for (i in v) {
        ...
        v[i] = n;
    }

i is an integer-string and we didn't optimize that case before but maybeGuardInt32Index handles it fine \o/. There might be another win when we convert the Ion IC to CacheIR.
https://hg.mozilla.org/mozilla-central/rev/96454c1e87ee
(Reporter)

Comment 9

3 months ago
Comment on attachment 8836025 [details] [diff] [review]
Part 2 - Setting Dense/UnboxedArray holes

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

Nice work! I am sure handling in-bounds sets is going to be a win somewhere. The whole disabled forever situation for TypedObjects and even more so for unboxed-arrays is really starting to irritate me as well.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1170,5 @@
> +    Label doStore;
> +    if (handleAdd) {
> +        // If index == initLength, increment initLength.
> +        Label inBounds;
> +        masm.branch32(Assembler::NotEqual, initLength, index, &inBounds);

So basically the handleAdd case can also handle the setting a hole case. It makes sense, but took me a while to understand that.

::: js/src/jit/CacheIR.cpp
@@ +2274,5 @@
> +            (clasp->getAddProperty() ||
> +             clasp->getResolve() ||
> +             clasp->getOpsLookupProperty() ||
> +             clasp->getSetProperty() ||
> +             clasp->getOpsSetProperty()))

Am I missing something or did we just not do this before?

@@ +2370,5 @@
> +
> +    // Type inference uses JSID_VOID for the element types.
> +    setUpdateStubInfo(nobj->group(), JSID_VOID);
> +
> +    trackAttached("StoreDenseElementHole");

Would be nice to nice to log something else for |isAdd|, but not crucial.
Attachment #8836025 - Flags: review?(evilpies) → review+
(Assignee)

Comment 10

3 months ago
(In reply to Tom Schuster [:evilpie] from comment #9)
> Am I missing something or did we just not do this before?

We didn't. At least some of these hooks are not used at all or are not used for dense elements, and attaching the stub after the SetElement call probably helped avoid some problems with lookup/resolve hooks. I added the checks just to be safe.

> Would be nice to nice to log something else for |isAdd|, but not crucial.

I'll change it to: isAdd ? "AddDenseElement" : "StoreDenseElementHole"

Comment 11

3 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f60b9d2c5bd
part 2 - Port Baseline stubs for adding dense/unboxed elements to CacheIR. r=evilpie
https://hg.mozilla.org/mozilla-central/rev/3f60b9d2c5bd
(Assignee)

Comment 13

3 months ago
Created attachment 8837569 [details] [diff] [review]
Part 3 - Setting TypedArray/TypedObject elements

We used to set a flag on the typed array stub if the access was out-of-bounds (this was added for a broken benchmark that was fixed since then I think), I changed that to set the flag on the fallback stub. I also renamed the hasArrayWriteHole flag to hasDenseAdd.

It might be nicer to add separate CacheIR instructions for the RHS double/int32 check/conversion and for the uint8 clamping, so we can share more code with Ion maybe, but we can always do that later when we know what the Ion code for this looks like.

This was the last Baseline SetProp/SetElem stub so we can finally start working on the parts we need for Ion.
Attachment #8837569 - Flags: review?(evilpies)
(Assignee)

Comment 14

3 months ago
Oh and we also used to unlink non-OOB typed array stubs when we attached an OOB stub. I don't think we need to keep that behavior as OOB should be pretty rare for typed arrays and even if it happens we just have an extra stub but that's not too bad.
(Reporter)

Comment 15

3 months ago
Comment on attachment 8837569 [details] [diff] [review]
Part 3 - Setting TypedArray/TypedObject elements

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

\o/ And we are done porting Baseline GetProp/SetProp. 

I wonder if any benchmark we track actually hits the TypedArray OOB case, because Chakra and JSC supposedly throw on that.

::: js/src/jit/BaselineIC.cpp
@@ +1099,2 @@
>  void
> +BaselineStoreToTypedArray(JSContext* cx, MacroAssembler& masm, Scalar::Type type,

Can't we move this?
Attachment #8837569 - Flags: review?(evilpies) → review+
(Assignee)

Comment 16

3 months ago
(In reply to Tom Schuster [:evilpie] from comment #15)
> I wonder if any benchmark we track actually hits the TypedArray OOB case,
> because Chakra and JSC supposedly throw on that.

Good point, I can remove it if you want? It would simplify this patch quite a bit.

> ::: js/src/jit/BaselineIC.cpp
> > +BaselineStoreToTypedArray(JSContext* cx, MacroAssembler& masm, Scalar::Type type,
> 
> Can't we move this?

Yes. I didn't do that here to make it easier to review, but at some point we should reorganize the different IC files.
Flags: needinfo?(evilpies)
(Assignee)

Comment 17

3 months ago
We discussed this on IRC and we'll keep this for now because both Chakra and JSC don't throw on OOB sets.
(Assignee)

Updated

3 months ago
Flags: needinfo?(evilpies)

Comment 18

3 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98eeda9744e6
part 3 - Port Baseline TypedArray/TypedObject SetElem stub to CacheIR. r=evilpie
(Assignee)

Updated

3 months ago
Keywords: leave-open
Little bit late, but marked this p1.
Priority: -- → P1

Comment 20

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98eeda9744e6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.