860997 - PJS: ParallelGetPropertyIC should keep a set of stubbed shapes instead of objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

The guard should be on shape identity, not object identity.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: patch + test

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 736563 [details] [diff] [review]
patch + test

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

Be aware that checking the shape is not enough, as multiple objects can have the same shape without having the same prototype chain.  This implementation is still be safe as it will fallback on the pure getter of the parallel IC, but this should be avoided.

::: js/src/ion/IonCaches.cpp
@@ +1426,5 @@
>              // Check if we have already stubbed the current object to avoid
>              // attaching a duplicate stub.
>              if (!cache.initStubbedObjects(cx))
>                  return TP_FATAL;
> +            ShapeSet::AddPtr p = cache.stubbedShapes()->lookupForAdd(obj->lastProperty());

The shape does not account for the prototype chain.  So checking the shape is probably not enough.

::: js/src/jit-test/tests/parallelarray/parallel-getproperty-ic.js
@@ +20,5 @@
> +
> +  var o1 = { foo: 0 };
> +  var o2 = { foo: 0, bar: '' };
> +  var o3 = { foo: 0, bar: '', baz: function () { } };
> +  f(o1); f(o2); f(o3); f(o2); f(o1);

nit: Add a 4th object identical to o1, with a different value of foo, but with the same shape.

@@ +25,5 @@
> +}
> +
> +if (getBuildConfiguration().parallelJS) {
> +  testICProto();
> +  testICMultiple();

Add a test case with 2 objects with different proto but identical shape.

function A () {
  this.bar = 1;
}
A.prototype.foo = "a";
A.prototype.a = true;
var x = new A;

function B () {
  this.bar = 2;
}
B.prototype.foo = "b";
A.prototype.b = true;
var y = new B;

// Should have stubs for each shape & proto-chain.
  … o.foo …

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Comment on attachment 736563 [details] [diff] [review]
> patch + test
> 
> Review of attachment 736563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Be aware that checking the shape is not enough, as multiple objects can have
> the same shape without having the same prototype chain.  This implementation
> is still be safe as it will fallback on the pure getter of the parallel IC,
> but this should be avoided.
> 
> ::: js/src/ion/IonCaches.cpp
> @@ +1426,5 @@
> >              // Check if we have already stubbed the current object to avoid
> >              // attaching a duplicate stub.
> >              if (!cache.initStubbedObjects(cx))
> >                  return TP_FATAL;
> > +            ShapeSet::AddPtr p = cache.stubbedShapes()->lookupForAdd(obj->lastProperty());
> 
> The shape does not account for the prototype chain.  So checking the shape
> is probably not enough.

You're right, it's not. This is a check to avoid generating duplicate (like you said, not incorrect, just kind of undesirable) stubs when multiple objects contend on the same IC in parallel. I don't think the extra complexity of keeping track of the prototype chain is worth the rarity with which it occurs.

> 
> ::: js/src/jit-test/tests/parallelarray/parallel-getproperty-ic.js
> @@ +20,5 @@
> > +
> > +  var o1 = { foo: 0 };
> > +  var o2 = { foo: 0, bar: '' };
> > +  var o3 = { foo: 0, bar: '', baz: function () { } };
> > +  f(o1); f(o2); f(o3); f(o2); f(o1);
> 
> nit: Add a 4th object identical to o1, with a different value of foo, but
> with the same shape.
> 
> @@ +25,5 @@
> > +}
> > +
> > +if (getBuildConfiguration().parallelJS) {
> > +  testICProto();
> > +  testICMultiple();
> 
> Add a test case with 2 objects with different proto but identical shape.
> 
> function A () {
>   this.bar = 1;
> }
> A.prototype.foo = "a";
> A.prototype.a = true;
> var x = new A;
> 
> function B () {
>   this.bar = 2;
> }
> B.prototype.foo = "b";
> A.prototype.b = true;
> var y = new B;
> 
> // Should have stubs for each shape & proto-chain.
>   … o.foo …

Will do.

Comment 4

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8904f1948e49

try: https://tbpl.mozilla.org/?tree=Try&rev=cf3ef1bd6177

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8904f1948e49