928928 - BaselineCompiler: Add missing property GetProp stub

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Not having this IC stub is the main reason why we're slower than V8 and JSC on the "[coll [1 2 3]] (satisfies? ISeq coll)" Clojurescript benchmark.

The GETPROP is inside this *huge* global script and even if we can Ion-compile it off-thread, it will take a while so we need a Baseline stub for this to not spend most of our time in the VM.

Comment 1

[Jan de Mooij [:jandem]]

Also noticed this on Octane-TypeScript (bug 935929). Real-world code does |if (obj.prop)| all the time; I want to fix this soonish.

Comment 2

[Kannan Vijayan [:djvj]]

I have a quick test patch written for this.  Not quite catching the optimized case yet but it should be largely there.

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: add-baseline-no-such-prop-stub.patch

Try run looks ok so far: https://tbpl.mozilla.org/?tree=Try&rev=650603d59e39

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8446128 [details] [diff] [review]
add-baseline-no-such-prop-stub.patch

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

This sounds good, just some clean-up avoid weird offsetOf and and runtime checks.

Can you also add a test case to check all the proto chain depth up to 10?

::: js/src/jit/BaselineIC.cpp
@@ +3354,5 @@
>  static bool
> +CheckHasNoSuchProperty(JSContext *cx, HandleObject obj, HandlePropertyName name,
> +                       MutableHandleObject lastProto, size_t *protoChainDepthOut)
> +{
> +    JS_ASSERT(protoChainDepthOut != nullptr);

nit: no need to assert, as this will crash anyway.

@@ +6773,5 @@
> +
> +    if (!GetProtoShapes(obj_, protoChainDepth_, &shapes))
> +        return nullptr;
> +
> +    JS_STATIC_ASSERT(ICGetProp_NativeDoesNotExist::MAX_PROTO_CHAIN_DEPTH == 8);

nit: s/JS_/MOZ_/

@@ +6806,5 @@
> +    Register scratch = regs.takeAny();
> +
> +    // Unbox and guard against old shape.
> +    Register objReg = masm.extractObject(R0, ExtractTemp0);
> +    masm.loadPtr(Address(BaselineStubReg, ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(0)),

Can you add some debug-only generated code to check that the protoChainDepth_ is equal to the
  loadPtr(Address(BaselineStubReg, ICGetProp_NativeDoesNotExist::offsetOfProtoChainDepth(0)))

@@ +6815,5 @@
> +    // Check the proto chain.
> +    for (size_t i = 0; i < protoChainDepth_; i++) {
> +        masm.loadObjProto(i == 0 ? objReg : protoReg, protoReg);
> +        masm.branchTestPtr(Assembler::Zero, protoReg, protoReg, &failure);
> +        size_t shapeOffset = ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(i + 1);

Move offsetOfShape to ICGetProp_NativeDoesNotExist, as it is independent and add an assertion to check that the offset of the shape is indeed independent of the protoChainDepth.

  MOZ_ASSERT(shapeOffset == ICGetProp_NativeDoesNotExistImpl<8>::offsetOfShape(i + 1));

Comment 5

[Kannan Vijayan [:djvj]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Comment on attachment 8446128 [details] [diff] [review]
> add-baseline-no-such-prop-stub.patch
> 
> Review of attachment 8446128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This sounds good, just some clean-up avoid weird offsetOf and and runtime
> checks.
> 
> Can you also add a test case to check all the proto chain depth up to 10?
> 
> ::: js/src/jit/BaselineIC.cpp
> @@ +3354,5 @@
> >  static bool
> > +CheckHasNoSuchProperty(JSContext *cx, HandleObject obj, HandlePropertyName name,
> > +                       MutableHandleObject lastProto, size_t *protoChainDepthOut)
> > +{
> > +    JS_ASSERT(protoChainDepthOut != nullptr);
> 
> nit: no need to assert, as this will crash anyway.

I'd like to leave it in because not all paths to return will trigger that crash.

> 
> @@ +6773,5 @@
> > +
> > +    if (!GetProtoShapes(obj_, protoChainDepth_, &shapes))
> > +        return nullptr;
> > +
> > +    JS_STATIC_ASSERT(ICGetProp_NativeDoesNotExist::MAX_PROTO_CHAIN_DEPTH == 8);
> 
> nit: s/JS_/MOZ_/

There are no current uses of MOZ_STATIC_ASSERT in the js subtree.  Does it even exist?  I do see that we're converting regular JS_ASSERTs to MOZ_ASSERTS, though.  However, given that there are many JS_ASSERTs still remaining in the subtree, we should have a single patch that changes them all over.

> 
> @@ +6806,5 @@
> > +    Register scratch = regs.takeAny();
> > +
> > +    // Unbox and guard against old shape.
> > +    Register objReg = masm.extractObject(R0, ExtractTemp0);
> > +    masm.loadPtr(Address(BaselineStubReg, ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(0)),
> 
> Can you add some debug-only generated code to check that the
> protoChainDepth_ is equal to the
>   loadPtr(Address(BaselineStubReg,
> ICGetProp_NativeDoesNotExist::offsetOfProtoChainDepth(0)))

Good suggestion.  Done.

> @@ +6815,5 @@
> > +    // Check the proto chain.
> > +    for (size_t i = 0; i < protoChainDepth_; i++) {
> > +        masm.loadObjProto(i == 0 ? objReg : protoReg, protoReg);
> > +        masm.branchTestPtr(Assembler::Zero, protoReg, protoReg, &failure);
> > +        size_t shapeOffset = ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(i + 1);
> 
> Move offsetOfShape to ICGetProp_NativeDoesNotExist, as it is independent and
> add an assertion to check that the offset of the shape is indeed independent
> of the protoChainDepth.

This is problematic.  The base ICGetProp_NativeDoesNotExist doesn't store the shapes array.  It can cast itself to the appropriate parameterized Impl type, but then the method code would have to be moved into the .cpp file (since the Impl type has to be declared after the base type, and thus header-implemented functions in the base class can't talk about the fields of the subclass.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Kannan Vijayan [:djvj] from comment #5)
> (In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > Comment on attachment 8446128 [details] [diff] [review]
> > add-baseline-no-such-prop-stub.patch
> > 
> > Review of attachment 8446128 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This sounds good, just some clean-up avoid weird offsetOf and and runtime
> > checks.
> > 
> > Can you also add a test case to check all the proto chain depth up to 10?
> > 
> > ::: js/src/jit/BaselineIC.cpp
> > @@ +3354,5 @@
> > >  static bool
> > > +CheckHasNoSuchProperty(JSContext *cx, HandleObject obj, HandlePropertyName name,
> > > +                       MutableHandleObject lastProto, size_t *protoChainDepthOut)
> > > +{
> > > +    JS_ASSERT(protoChainDepthOut != nullptr);
> > 
> > nit: no need to assert, as this will crash anyway.
> 
> I'd like to leave it in because not all paths to return will trigger that
> crash.
> 
> > 
> > @@ +6773,5 @@
> > > +
> > > +    if (!GetProtoShapes(obj_, protoChainDepth_, &shapes))
> > > +        return nullptr;
> > > +
> > > +    JS_STATIC_ASSERT(ICGetProp_NativeDoesNotExist::MAX_PROTO_CHAIN_DEPTH == 8);
> > 
> > nit: s/JS_/MOZ_/
> 
> There are no current uses of MOZ_STATIC_ASSERT in the js subtree.  Does it
> even exist?  I do see that we're converting regular JS_ASSERTs to
> MOZ_ASSERTS, though.  However, given that there are many JS_ASSERTs still
> remaining in the subtree, we should have a single patch that changes them
> all over.
> 
> > 
> > @@ +6806,5 @@
> > > +    Register scratch = regs.takeAny();
> > > +
> > > +    // Unbox and guard against old shape.
> > > +    Register objReg = masm.extractObject(R0, ExtractTemp0);
> > > +    masm.loadPtr(Address(BaselineStubReg, ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(0)),
> > 
> > Can you add some debug-only generated code to check that the
> > protoChainDepth_ is equal to the
> >   loadPtr(Address(BaselineStubReg,
> > ICGetProp_NativeDoesNotExist::offsetOfProtoChainDepth(0)))
> 
> Good suggestion.  Done.
> 
> > @@ +6815,5 @@
> > > +    // Check the proto chain.
> > > +    for (size_t i = 0; i < protoChainDepth_; i++) {
> > > +        masm.loadObjProto(i == 0 ? objReg : protoReg, protoReg);
> > > +        masm.branchTestPtr(Assembler::Zero, protoReg, protoReg, &failure);
> > > +        size_t shapeOffset = ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(i + 1);
> > 
> > Move offsetOfShape to ICGetProp_NativeDoesNotExist, as it is independent and
> > add an assertion to check that the offset of the shape is indeed independent
> > of the protoChainDepth.
> 
> This is problematic.  The base ICGetProp_NativeDoesNotExist doesn't store
> the shapes array.  It can cast itself to the appropriate parameterized Impl
> type, but then the method code would have to be moved into the .cpp file
> (since the Impl type has to be declared after the base type, and thus
> header-implemented functions in the base class can't talk about the fields
> of the subclass.

Sorry, I meant to wrap this function with ICGetProp_NativeDoesNotExist::offsetOfShape, such as we do not ask about the weird iteration on an array which has 0 elements, and assert and comment that they have identical offset idenpendently of the number of shapes.

Comment 7

[Kannan Vijayan [:djvj]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Sorry, I meant to wrap this function with
> ICGetProp_NativeDoesNotExist::offsetOfShape, such as we do not ask about the
> weird iteration on an array which has 0 elements, and assert and comment
> that they have identical offset idenpendently of the number of shapes.

Responded on IRC to this, but I thought I'd post a comment too.

I don't understand the gain from doing this.  To properly wrap the DoesNotExistImpl<N>::offsetOfShape methods with a method in the base class, the base class implementation would look something like this:

  static size_t ICGetProp_NativeDoesNotExist::offsetOfShape(size_t idx, size_t protoChainDepth) {
    switch(protoChainDepth) {
      case 0: return ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(idx);
      case 1: ...
      ...
      case 8: ...
    }
  }

We'll have to manually pass in the proto chain depth, then switch on it (so it can be re-introduced as a constant for use as a template parameter), and then call the actual method.

The benefit gained by an extra ASSERT on this (it's already ASSERTED all over the place) seems negligible compared to the convlutedness and confusion of the implementation.

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: add-baseline-no-such-prop-stub.patch

Revised patch with MOZ_ASSERT instead of JS_ASSERT, and extra debug-mode-only JIT instrumentation.  Tests coming separately.

Comment 9

[Kannan Vijayan [:djvj]]

Attachment: add-baseline-no-such-prop-stub-tests.patch

The tests.  The test file itself is generated by a test generator that's included in this patch.

The generator generates the following tests:

For every proto chain length N where N in [0..9],
    For every depth D where D in [0..N-1],
        Create an object with a proto chain of length N
        Perform get operation on nonexistant property.
        Set the property on the Dth prototype (starting from the derived object)
        Re-perform get operation on now existant property.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Kannan Vijayan [:djvj] from comment #7)
> (In reply to Nicolas B. Pierron [:nbp] from comment #6)
> > Sorry, I meant to wrap this function with
> > ICGetProp_NativeDoesNotExist::offsetOfShape, such as we do not ask about the
> > weird iteration on an array which has 0 elements, and assert and comment
> > that they have identical offset idenpendently of the number of shapes.
> 
> I don't understand the gain from doing this.  To properly wrap the
> DoesNotExistImpl<N>::offsetOfShape methods with a method in the base class,
> the base class implementation would look something like this:

My suggestion was not to dispatch but to avoid the confusion of using the address of non-existing elements without a commant & assertion, such as:

  static size_t ICGetProp_NativeDoesNotExist::offsetOfShape(size_t idx, size_t protoChainDepth) {
    // The offset of shape are the same for all stubs, except that they are not valid for all of them.
    MOZ_ASSERT(ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(idx) ==
               ICGetProp_NativeDoesNotExistImpl<8>::offsetOfShape(idx));
    return ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(idx);
  }

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8446709 [details] [diff] [review]
add-baseline-no-such-prop-stub.patch

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

::: js/src/jit/BaselineIC.cpp
@@ +6819,5 @@
> +    {
> +        Label ok;
> +        masm.load16ZeroExtend(Address(BaselineStubReg, ICStub::offsetOfExtra()), scratch);
> +        masm.branch32(Assembler::Equal, scratch, Imm32(protoChainDepth_), &ok);
> +        masm.breakpoint();

nit: use masm.assumeUnreachable("...");

@@ +6838,5 @@
> +    // Check the proto chain.
> +    for (size_t i = 0; i < protoChainDepth_; i++) {
> +        masm.loadObjProto(i == 0 ? objReg : protoReg, protoReg);
> +        masm.branchTestPtr(Assembler::Zero, protoReg, protoReg, &failure);
> +        size_t shapeOffset = ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(i + 1);

see comment 10.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8446712 [details] [diff] [review]
add-baseline-no-such-prop-stub-tests.patch

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

Nice :)

::: js/src/jit-test/etc/generate-nosuchproperty-tests.js
@@ +17,5 @@
> +    print("    return sum;");
> +    print("}");
> +    print("function " + testFuncName + "() {");
> +    print("    var n = " + n + ", d = " + d + ";");
> +    print("    var obj = createTower(n);");

tip:

function testChain_nnnn_dddd() {
  var n = nnnn, d = dddd;
  var obj = createTower(n);
  assertEq(runChain_nnnn_dddd(obj), NaN);
  updateChain(obj, d, 'foo', 9);
  assertEq(runChain_nnnn_dddd(obj), 900);
}

print(uneval(testChain_nnnn_dddd).replace("nnnn", n).replace("dddd", d));

Comment 13

[Kannan Vijayan [:djvj]]

Attachment: add-baseline-no-such-prop-stub.patch

Attaching final patch since there were nontrivial changes.  Forwarding nbp r+ from previous patch.

Comment 14

[Kannan Vijayan [:djvj]]

Attachment: add-baseline-no-such-prop-stub-tests.patch

Attaching final patch.  Forwarding nbp r+ from previous patch.

Comment 15

[Kannan Vijayan [:djvj]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> (In reply to Kannan Vijayan [:djvj] from comment #7)
> > (In reply to Nicolas B. Pierron [:nbp] from comment #6)
> > > Sorry, I meant to wrap this function with
> > > ICGetProp_NativeDoesNotExist::offsetOfShape, such as we do not ask about the
> > > weird iteration on an array which has 0 elements, and assert and comment
> > > that they have identical offset idenpendently of the number of shapes.
> > 
> > I don't understand the gain from doing this.  To properly wrap the
> > DoesNotExistImpl<N>::offsetOfShape methods with a method in the base class,
> > the base class implementation would look something like this:
> 
> My suggestion was not to dispatch but to avoid the confusion of using the
> address of non-existing elements without a commant & assertion, such as:
> 
>   static size_t ICGetProp_NativeDoesNotExist::offsetOfShape(size_t idx,
> size_t protoChainDepth) {
>     // The offset of shape are the same for all stubs, except that they are
> not valid for all of them.
>     MOZ_ASSERT(ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(idx) ==
>                ICGetProp_NativeDoesNotExistImpl<8>::offsetOfShape(idx));
>     return ICGetProp_NativeDoesNotExistImpl<0>::offsetOfShape(idx);
>   }

Cool, that makes sense.  Done.

Comment 16

[Kannan Vijayan [:djvj]]

(In reply to Nicolas B. Pierron [:nbp] from comment #12)
> tip:
> 
> function testChain_nnnn_dddd() {
>   var n = nnnn, d = dddd;
>   var obj = createTower(n);
>   assertEq(runChain_nnnn_dddd(obj), NaN);
>   updateChain(obj, d, 'foo', 9);
>   assertEq(runChain_nnnn_dddd(obj), 900);
> }
> 
> print(uneval(testChain_nnnn_dddd).replace("nnnn", n).replace("dddd", d));


nice suggestion!  Done.

Latest try: https://tbpl.mozilla.org/?tree=Try&rev=8493f157470a

Comment 17

[Kannan Vijayan [:djvj]]

Ok that try run was on top of a busted build.  New try run is looking OK to start: https://tbpl.mozilla.org/?tree=Try&rev=df4f696948e5

Comment 18

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95ce6d0c7993

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/95ce6d0c7993