1095952 - Optimize accesses to known inline or outline typed objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently, when accessing a typed object we emit code to compute its elements pointer, whether it is inline or outline.  This can be improved substantially: in cases when the object is known to be outline we don't need to test its class before loading its data pointer, and in cases where the object is known to be inline we don't need to compute the elements at all and can instead perform the access based on the object pointer.  The attached patch makes these changes to typed object code generation.  It also changes typed object code generation to use LinearSums where possible rather than raw MDefinitions; these are easier to pattern match and reason about when we generate MIR nodes to access the typed object's contents.

Asking for review from both nmatsakis and jandem as this makes changes to a lot of MIR nodes used by both normal and typed objects.

Comment 1

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8519485 [details] [diff] [review]
patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +5102,5 @@
>  {
>      Register obj = ToRegister(lir->object());
>      Register out = ToRegister(lir->output());
>  
> +    if (lir->mir()->definitelyOutline()) {

Do you plan to optimize the "definitely inline" case as well?

::: js/src/jit/MIR.h
@@ +7463,5 @@
> +    bool definitelyOutline() const {
> +        return definitelyOutline_;
> +    }
> +    bool congruentTo(const MDefinition *ins) const {
> +        if (!ins->isTypedObjectElements())

This is a total bikeshed, but it'd be shorter if you did

if (!congruentIfOperandsEqual(other))
    return false;

and then did the other stuff, I think, since it handles the "same kind of instruction" case and so on.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8519485 [details] [diff] [review]
patch

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

::: js/src/jit/IonBuilder.cpp
@@ +9542,5 @@
>      MDefinition *fieldTypeObj = typeObjectForFieldFromStructType(type, fieldIndex);
>  
> +    LinearSum byteOffset(alloc());
> +    if (!byteOffset.add(fieldOffset))
> +        setForceAbort();

Why don't we use |return abort("...");| here, like we do elsewhere in IonBuilder? It seems a bit confusing to have two different abort mechanisms.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Niko Matsakis [:nmatsakis] from comment #1)
> ::: js/src/jit/CodeGenerator.cpp
> @@ +5102,5 @@
> >  {
> >      Register obj = ToRegister(lir->object());
> >      Register out = ToRegister(lir->output());
> >  
> > +    if (lir->mir()->definitelyOutline()) {
> 
> Do you plan to optimize the "definitely inline" case as well?

Potentially, though for plain accesses we shouldn't be generating MTypedObjectElements for objects which are definitely inline, as these cases will be covered by the offset adjustments.

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Comment on attachment 8519485 [details] [diff] [review]
> patch
> 
> Review of attachment 8519485 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/IonBuilder.cpp
> @@ +9542,5 @@
> >      MDefinition *fieldTypeObj = typeObjectForFieldFromStructType(type, fieldIndex);
> >  
> > +    LinearSum byteOffset(alloc());
> > +    if (!byteOffset.add(fieldOffset))
> > +        setForceAbort();
> 
> Why don't we use |return abort("...");| here, like we do elsewhere in
> IonBuilder? It seems a bit confusing to have two different abort mechanisms.

The other setForceAbort calls are in infallible functions, and making them fallible would require making lots of other stuff fallible.  I used setForceAbort() here too for consistency.  FWIW I think this mechanism is much nicer to use than return-false-on-abort in IonBuilder, and could be used to clean up a lot of interfaces --- making them infallible, removing outparams, and so forth.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: beef up type policies

During further testing I found that the changes in this patch require changes to the type policies on involved instructions.  Previously we could just assert the input has MIRType_Elements, but if the input can be MIRType_Object it could be a phi and have its type changed during phi analysis (phis can't have MIRType_Elements.)

Comment 6

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d696d5bfb8cf

Comment 7

[Nicolas B. Pierron [:nbp]]

This issue likely caused a regression in Mandreel benchmark.

http://arewefastyet.com/#machine=28&view=single&suite=octane&subtest=Mandreel&start=1416264308&end=1416333145

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> This issue likely caused a regression in Mandreel benchmark.
> 
> http://arewefastyet.com/
> #machine=28&view=single&suite=octane&subtest=Mandreel&start=1416264308&end=14
> 16333145

Hmm, ok, I'll look at this later today.

Comment 9

[Brian Hackett [Laid off!]]

Attachment: patch

This should fix the mandreel regression.  The StoreTypedArrayPolicy type policy change was being applied to StoreTypedArrayElementStatic, which caused things to go haywire.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8524879 [details] [diff] [review]
patch

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

::: js/src/jit/TypePolicy.cpp
@@ +820,5 @@
>      MStoreTypedArrayElement *store = ins->toStoreTypedArrayElement();
>      MOZ_ASSERT(IsValidElementsType(store->elements(), store->offsetAdjustment()));
>      MOZ_ASSERT(store->index()->type() == MIRType_Int32);
>  
> +    SingleObjectPolicy::staticAdjustInputs(alloc, ins);

We should also do this for StoreTypedArrayHolePolicy below AFAICS, r=me with that.

Comment 11

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> ::: js/src/jit/TypePolicy.cpp
> @@ +820,5 @@
> >      MStoreTypedArrayElement *store = ins->toStoreTypedArrayElement();
> >      MOZ_ASSERT(IsValidElementsType(store->elements(), store->offsetAdjustment()));
> >      MOZ_ASSERT(store->index()->type() == MIRType_Int32);
> >  
> > +    SingleObjectPolicy::staticAdjustInputs(alloc, ins);
> 
> We should also do this for StoreTypedArrayHolePolicy below AFAICS, r=me with
> that.

This isn't necessary as the elements input to MStoreTypedArrayElementHole definitely has MIRType_Elements, whose values won't see type changes due to phi type analysis per comment 5.  The changes in the earlier patches in this bug only apply to MStoreTypedArrayElement, as this is the only instruction used by typed object code.

Comment 12

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c14cd1b976ef

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d696d5bfb8cf

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c14cd1b976ef