Closed Bug 1095952 Opened 10 years ago Closed 10 years ago

Optimize accesses to known inline or outline typed objects

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(3 files)

Attached patch patchSplinter Review
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.
Attachment #8519485 - Flags: review?(nmatsakis)
Attachment #8519485 - Flags: review?(jdemooij)
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.
Attachment #8519485 - Flags: review?(nmatsakis) → review+
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.
Attachment #8519485 - Flags: review?(jdemooij) → review+
(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.
(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.
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.)
Assignee: nobody → bhackett1024
Attachment #8523571 - Flags: review?(jdemooij)
Attachment #8523571 - Flags: review?(jdemooij) → review+
(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.
Attached patch patchSplinter Review
This should fix the mandreel regression.  The StoreTypedArrayPolicy type policy change was being applied to StoreTypedArrayElementStatic, which caused things to go haywire.
Attachment #8524879 - Flags: review?(jdemooij)
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.
Attachment #8524879 - Flags: review?(jdemooij) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/d696d5bfb8cf
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.