Closed Bug 1337024 Opened 3 years ago Closed 3 years ago

Convert SETELEM dense/typed array IC to CacheIR

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: evilpie, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

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: nobody → jdemooij
Status: NEW → ASSIGNED
Depends on: 1337871
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)
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+
(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.
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)
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 :/
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
Keywords: leave-open
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.
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+
(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"
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
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)
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.
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+
(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)
We discussed this on IRC and we'll keep this for now because both Chakra and JSC don't throw on OOB sets.
Flags: needinfo?(evilpies)
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
Keywords: leave-open
Little bit late, but marked this p1.
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/98eeda9744e6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.