Optimize polymorphic property gets/sets accessing the same slot

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Octane-Gameboy has a ton of GetPropertyPolymorphic instructions with two different object shapes but in many cases the property is in the same slot.

It seems like in this case we don't want GetPropertyPolymorphic/SetPropertyPolymorphic but a (new) MGuardShapePolymorphic instruction that guards on multiple shapes followed by a LoadSlot/StoreSlot.

Then we can hoist/coalesce the shape guards separately and also emit smaller code.
Posted patch PatchSplinter Review
This is a small win on Octane-Gameboy; most of the property accesses there can be optimized this way.

The patch also gets rid of some unnecessary branches for GetPropertyPolymorphic/SetPropertyPolymorphic.
Attachment #8411810 - Flags: review?(bhackett1024)
Comment on attachment 8411810 [details] [diff] [review]
Patch

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

::: js/src/jit/IonBuilder.cpp
@@ +8870,5 @@
> +    for (size_t i = 0; i < shapes.length(); i++) {
> +        Shape *objShape = shapes[i];
> +        Shape *shape = objShape->searchLinear(id);
> +        MOZ_ASSERT(shape);
> +        if (!propShapes.append(shape))

This looks like it can be infallibleAppend.
Attachment #8411810 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/d1e4a93e5b6c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(In reply to Jan de Mooij [:jandem] from comment #1)
> This is a small win on Octane-Gameboy

17% win according to AWFY on all but the ARMs.
That places it almost the same as V8 on Mac x86! Well done!
Comment on attachment 8411810 [details] [diff] [review]
Patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +1513,5 @@
> +                return false;
> +            }
> +        } else {
> +            masm.branchPtr(Assembler::NotEqual, scratch, ImmGCPtr(mir->objShape(i)), &next);
> +        }

I thinks this could be handled a bit more elegant:

> for (size_t i = 0; i < mir->numShapes(); i++) {
>     Label next;
>     masm.branchPtr(Assembler::NotEqual, scratch, ImmGCPtr(mir->objShape(i)), &next);
>
>     ...
>
>     if (i != mir->numShapes() - 1) {
>         masm.jump(&done);
>         masm.bind(&next);
>     } else {
>         // Processing last shape, so there is no next. Bail in that case.
>         if (!bailoutFrom(&next, ins->snapshot()))
>             return false;
>     }
> }

@@ -1525,5 @@
>      }
>  
> -    // Bailout if no shape matches.
> -    if (!bailout(ins->snapshot()))
> -        return false;

When "mir->numShapes() == 0" we should bail. In the patched version we don't do that anymore! Is there a reason we don't need to bailout in that case? Or is this oversight?

@@ -1586,5 @@
>      }
>  
> -    // Bailout if no shape matches.
> -    if (!bailout(ins->snapshot()))
> -        return false;

Same here?

@@ +1753,5 @@
> +        } else {
> +            masm.branchPtr(Assembler::Equal, temp, ImmGCPtr(shape), &done);
> +        }
> +    }
> +

Same here?
(In reply to Hannes Verschore [:h4writer] from comment #6)
> I thinks this could be handled a bit more elegant:

Yeah, we could use bailoutFrom but it's pretty straight-forward either way.

> When "mir->numShapes() == 0" we should bail. In the patched version we don't
> do that anymore! Is there a reason we don't need to bailout in that case? Or
> is this oversight?

It makes no sense for numShapes to be <= 1, we assert it's > 1.
You need to log in before you can comment on or make changes to this bug.