822989 - BaselineCompiler: Add optimized GETPROP stub

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 2

[Kannan Vijayan [:djvj]]

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.

Comment 3

[Jan de Mooij [:jandem]]

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).