851792 - IonMonkey: add stubs in IonCache for TypedArray with JSAtom as index

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

In bug 851552 we disable the cache, to decrease the time, when hitting a case that isn't stubbed by our caches. For JPEG2000 we know which stub is hit and isn't implemented. Therefore we can get an even bigger increase by adding this stub.

We need to add a stub for TypedArray access that uses an JSAtom as index.

Doing this improved the benchmark from 7.8s to 5.4s.
That is a 44% improvement.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 726708 [details] [diff] [review]
Patch

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

Looks great! Comments below are mostly nits. However, using a string index is not very common so this needs tests. Can you add some and make sure they hit GetIndexFromString with --ion-eager?

It would be good to have at least tests for: out-of-bounds access, constant string index and doubles like "3.5" or "NaN".

::: js/src/ion/IonCaches.cpp
@@ +1624,5 @@
> +        JSString *string = idval.toString();
> +        JS_ASSERT(string->isAtom());
> +        JSAtom *atom = &string->asAtom();
> +        uint32_t i;
> +        JS_ASSERT(atom->isIndex(&i) && i != UINT32_MAX);

Can you just use JS_ASSERT(GetIndexFromString(idval.toString()) != UINT32_MAX); here?

@@ +1656,5 @@
> +        ignore.add(indexReg);
> +        masm.PopRegsInMaskIgnore(RegisterSet::Volatile(), ignore);
> +
> +        masm.cmp32(indexReg, Imm32(UINT32_MAX));
> +        masm.j(Assembler::Equal, &failures);

Nit: masm.branch32(Assembler::Equal, indexReg, Imm32(UINT32_MAX), &failures);

@@ +1759,5 @@
> +                (idval.isString() && GetIndexFromString(idval.toString()) != UINT32_MAX))
> +            {
> +                int arrayType = TypedArray::type(obj);
> +                bool floatOutput = arrayType == TypedArray::TYPE_FLOAT32 ||
> +                    arrayType == TypedArray::TYPE_FLOAT64;

Nit: fix indentation of this line to how it was previously.

::: js/src/ion/VMFunctions.cpp
@@ +554,5 @@
> +    // Masks the return value UINT32_MAX as failure to get the index.
> +    // I.e. it is impossible to distinguish between failing to get the index
> +    // or the actual index UINT32_MAX.
> +
> +    uint32_t index;

Nit: move this down a few lines, right before/after the "JSAtom *atom =" line.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Patch

Fixed the nits + added the testcase (based on tests/ion/typed-arrays-1.js)

Comment 4

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1330edb0de29

Increases performance from 5.6s to 4.8s

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1330edb0de29