840696 - IonMonkey: Octane, pdfjs.js:16635: Produce IC for get elem of mixed arrays & typed arrays.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently we are producing a CallGetElement to extract elements out-of the stream buffer because we cannot either match an Array or a Typed Array.  Making an IC will improve our performance by at least 5% on PdfJS (estimate made by modifying the decrypt function).

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: [part 1] Enable GetElem IC if typed arrays & arrays are mix-up.

Fix the condition which prevent generating ICs for mixed aRrays and typed arrays.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: [part 2] Add typed array support to GetElem ICs.

This patch add support for typed arrays in GetElementIC.
It would apply correctly only on top of the patch attached to Bug 813823.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: [part 3] Specialize the return type of GetElem ICs.

This patch add another variant of the GetElementIC lowering to specialize the return type in order to avoid unboxing the value once it has been read out of typed arrays.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: [part 4] Avoid boxing the index for GetElement ICs.

Re-use the previous specialization of the returned type (to avoid adding 2 more variants) to also support the case where the index is a known Int32.

This patch tries to avoid duplicating the MIR to specialize the index by removing the inheritance made on TypePolicy of MGetElementCache.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 713593 [details] [diff] [review]
[part 3] Specialize the return type of GetElem ICs.

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

It looks like this patch doesn't use TypedOrValueRegister or loadTypedOrValue - is that intended? Right now I can't see how it would actually specialize the output register.

::: js/src/ion/IonBuilder.cpp
@@ +5329,5 @@
>      return resumeAfter(ins);
>  }
>  
> +static JSValueType
> +getGetElemKnownType(bool needsHoleCheck, types::StackTypeSet *types)

nit: take off the initial "get"

::: js/src/ion/IonCaches.cpp
@@ +1581,5 @@
> +    // has been saved on the stack to hold the type such as we can test for
> +    // hole. The payload will remain the same and we won't need any unboxing.
> +    ValueOperand out;
> +    if (!output().hasValue()) {
> +#if defined(JS_NUNBOX32)

This will be the first instance of having to special case NUNBOX32 in IonCaches.cpp, which is a little unfortunate. Is there anything reasonable we can do to avoid this?

::: js/src/ion/RegisterSets.h
@@ +133,5 @@
>  class TypedOrValueRegister
>  {
>      // Type of value being stored.
>      MIRType type_;
> +    static const Register InvalidReg;

nit: You could probably just name this "Invalid".

@@ +193,5 @@
> +        if (hasValue())
> +            return valueReg().scratchReg();
> +        if (!typedReg().isFloat())
> +            return typedReg().gpr();
> +        return InvalidReg;

This should probably JS_NOT_REACHED(), based on how it's used.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 713594 [details] [diff] [review]
[part 4] Avoid boxing the index for GetElement ICs.

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

Cool.

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #5)
> Comment on attachment 713593 [details] [diff] [review]
> [part 3] Specialize the return type of GetElem ICs.
> 
> Review of attachment 713593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks like this patch doesn't use TypedOrValueRegister or
> loadTypedOrValue - is that intended? Right now I can't see how it would
> actually specialize the output register.

This is already use in the typed array IC which has already its output stored in a TypedOrValueRegister even if it was only used as a ValueOperand.

This patch unconditionally use a ValueOperand for generic IC in case the Array contains an hole, but it still load the payload correctly in the right register if the type is specialized.

> ::: js/src/ion/IonCaches.cpp
> @@ +1581,5 @@
> > +    // has been saved on the stack to hold the type such as we can test for
> > +    // hole. The payload will remain the same and we won't need any unboxing.
> > +    ValueOperand out;
> > +    if (!output().hasValue()) {
> > +#if defined(JS_NUNBOX32)
> 
> This will be the first instance of having to special case NUNBOX32 in
> IonCaches.cpp, which is a little unfortunate. Is there anything reasonable
> we can do to avoid this?

Yes, we can accept the 2 Register ValueOperand constructor on x64, which would just ignore the lhs.

> @@ +193,5 @@
> > +        if (hasValue())
> > +            return valueReg().scratchReg();
> > +        if (!typedReg().isFloat())
> > +            return typedReg().gpr();
> > +        return InvalidReg;
> 
> This should probably JS_NOT_REACHED(), based on how it's used.

Sounds fine, I just want to make this more generic otherwise we might have to guard against everything which is already done in this function.

Comment 8

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1ecef13ae9 (part 1)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3de5ec9de48a (part 2)

Mark as leave open, until I can land part 3 & 4.
dvander: Is there anything you want me to fix in part 3, or my reply is enough?

Comment 9

[Phil Ringnalda (:philor)]

Backed out for the sin of being on top of the Android-crashy bug 814823, in https://hg.mozilla.org/integration/mozilla-inbound/rev/9664c3c609a1

Comment 10

[Nicolas B. Pierron [:nbp]]

part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/c29100ac6809
part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/351045f6f123
part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0b2f1ab14d
part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/9058fd42c734

Try runs:
https://tbpl.mozilla.org/?tree=Try&rev=d200301c3087
https://tbpl.mozilla.org/?tree=Try&rev=a331e0e1413e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c29100ac6809
https://hg.mozilla.org/mozilla-central/rev/351045f6f123
https://hg.mozilla.org/mozilla-central/rev/ba0b2f1ab14d
https://hg.mozilla.org/mozilla-central/rev/9058fd42c734