Closed Bug 822989 Opened 10 years ago Closed 10 years ago

BaselineCompiler: Add optimized GETPROP stub

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Attached patch PatchSplinter Review
This patch adds two stubs, one if the property is found on the object itself and one if the property is on the prototype. In the latter case we have to guard on the holder's shape too, but fortunately we don't have to guard on any shapes between the object and holder so this doesn't need any variable-length data or loops.
Attachment #694283 - Flags: review?(kvijayan)
Comment on attachment 694283 [details] [diff] [review]
Patch

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

::: js/src/ion/BaselineIC.cpp
@@ +1381,5 @@
>      return true;
>  }
>  
>  static bool
> +IsCacheableProtoChain(JSObject *obj, JSObject *holder)

|IsCacheableProtoChain| shows up in Ion too.  And I think it's also stowed away in the JM code somewhere.  It seems to be a good candidate for moving directly onto the JSObject interfaces - e.g.:

obj->protoChainCacheableUpto(holder)

@@ +1402,5 @@
> +    return true;
> +}
> +
> +static bool
> +IsCacheableGetPropReadSlot(JSObject *obj, JSObject *holder, UnrootedShape shape)

Same as for |IsCacheableProtoChain|.  Let's move these into the VM proper.

@@ +1562,5 @@
> +ICGetPropNativeCompiler::generateStubCode(MacroAssembler &masm)
> +{
> +    Label failure;
> +    GeneralRegisterSet regs(availableGeneralRegs(0));
> +    regs.take(R0);

The above two lines can be shortened to:
GeneralRegisterSet regs(avilableGeneralRegs(1));

If that seems like it's too cryptic, then we should just remove the int argument to availableGeneralRegs() entirely, and explicitly take() R0 and/or R1 wherever appropriate.

@@ +1563,5 @@
> +{
> +    Label failure;
> +    GeneralRegisterSet regs(availableGeneralRegs(0));
> +    regs.take(R0);
> +    regs.takeUnchecked(BaselineTailCallReg);

This is not necessary.  BaselineTailCallReg is available in all architectures except ARM, and on ARM availableGeneralRegs() will exclude it.

@@ +1585,5 @@
> +        masm.loadPtr(Address(BaselineStubReg, ICGetProp_NativePrototype::offsetOfHolder()),
> +                     holderReg);
> +        masm.loadPtr(Address(BaselineStubReg, ICGetProp_NativePrototype::offsetOfHolderShape()),
> +                     scratch);
> +        masm.branchTestObjShape(Assembler::NotEqual, holderReg, scratch, &failure);

If the shape teleportation properties hold, do we need to check the holder object's shape?

If the holder object's shape changes, then it should imply a change to the inheritor object's shape, which means we won't get here because the shape check above would fail.
Attachment #694283 - Flags: review?(kvijayan) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/986680a2ffa0

(In reply to Kannan Vijayan [:djvj] from comment #2)
> 
> |IsCacheableProtoChain| shows up in Ion too.  And I think it's also stowed
> away in the JM code somewhere.  It seems to be a good candidate for moving
> directly onto the JSObject interfaces - e.g.:

It's slightly different though, unlike Ion we also check for uncacheable objects on the proto chain (at least for now, this case seems rare and doesn't show up in SS/V8).

> 
> The above two lines can be shortened to:
> GeneralRegisterSet regs(avilableGeneralRegs(1));

> This is not necessary.  BaselineTailCallReg is available in all
> architectures except ARM, and on ARM availableGeneralRegs() will exclude it.

Oops, right.

> If the shape teleportation properties hold, do we need to check the holder
> object's shape?
> 
> If the holder object's shape changes, then it should imply a change to the
> inheritor object's shape, which means we won't get here because the shape
> check above would fail.

Replied on IRC yesterday already, but for posterity: only the holder's shape changes (when setting a property on an object with the delegate shape flag, we walk up the proto chain and regenerate shapes).
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.