1438086 - Shape teleporting spring cleaning

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P2)

Description

[Ted Campbell [:tcampbell]]

The shape teleporting optimization weaves through the JITs but is quite opaque and hard to reason about. These patches attempt to refactor code and make reasoning more explicit. The hope is that it is becomes easier to evaluate what value this optimization still holds or add further tuning.

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1438086-Cleanup-IC-group-guards.patch

Refactor CacheIRWriter::guardGroup calls to add more assertions and be clearer about what the group guard is achieving.

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: 0002-Bug-1438086-Cleanup-shape-teleporting-optimization.patch

This patch tries to cleanup shape teleporting and make it easier to reason about.

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: 0002r2-Bug-1438086-Cleanup-shape-teleporting-optimization.patch

Fix missing error propagation.

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: 0002r3-Bug-1438086-Cleanup-shape-teleporting-optimization.patch

Only use GeneratePrototypeGuards when holder exists. The requirements for guarding a missing property are very different.

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: 0002r4-Bug-1438086-Cleanup-shape-teleporting-optimization.patch

Stop using GeneratePrototypeGuards when holder is null. Use toDictionary before clearing a shape flag. Rebase over file renames.

Comment 6

[Ted Campbell [:tcampbell]]

Try is looking green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9e2a8590cbcd6bbb5495573799b9d3e0063aa41

Comment 7

[Ted Campbell [:tcampbell]]

Comment on attachment 8950836 [details] [diff] [review]
0001-Bug-1438086-Cleanup-IC-group-guards.patch

This patch tries to clean up uses of guardGroup to better clarify the things a group guard ensures in different contexts and which of those behaviors we actually care about.

Comment 8

[Ted Campbell [:tcampbell]]

Comment on attachment 8951287 [details] [diff] [review]
0002r4-Bug-1438086-Cleanup-shape-teleporting-optimization.patch

This patch attempts to make shape teleporting optimization more explicit and easy to reason about.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8950836 [details] [diff] [review]
0001-Bug-1438086-Cleanup-IC-group-guards.patch

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

::: js/src/jit/CacheIR.h
@@ +556,5 @@
>          writeOpWithOperandId(CacheOp::GuardFunctionPrototype, rhs);
>          writeOperandId(protoId);
>          addStubField(slot, StubField::Type::RawWord);
>      }
>      void guardGroup(ObjOperandId obj, ObjectGroup* group) {

It is possible to make this private, to force everyone to use guardGroupFor* below?

::: js/src/vm/ObjectGroup.h
@@ +134,5 @@
>  
> +    bool hasUncacheableProto() const {
> +        /* We allow singletons to mutate their prototype after the group has
> +         * been created. If true, the JIT must re-check prototype even if group
> +         * has been seen before. */

Nit: these days we prefer //-comments

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8951287 [details] [diff] [review]
0002r4-Bug-1438086-Cleanup-shape-teleporting-optimization.patch

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

Heroic! Thanks a lot for making this sane.

::: js/src/jit/CacheIR.cpp
@@ +545,5 @@
>  GeneratePrototypeGuards(CacheIRWriter& writer, JSObject* obj, JSObject* holder, ObjOperandId objId)
>  {
> +    // Assuming target property is on |holder|, generate appropriate guards to
> +    // ensure |holder| is still on the prototype chain of |obj| and we haven't
> +    // introduced any covering definitions.

Nit: maybe s/covering/shadowing/ for consistency with code elsewhere

@@ +576,5 @@
> +    // change since the lookup of 'x' will stop at B.
> +    //
> +    // The second condition we must verify is that the prototype chain was not
> +    // mutated. The same mechanism as above is used. When the prototype link is
> +    // changed, we generate a new shape for the object. If the object who's

Nit: s/who's/whose/

@@ +588,5 @@
> +    // shapes (if they aren't themselves uncacheable).
> +    //
> +    // Let's consider the effect of UNCACHEABLE_PROTO flag on our example:
> +    // - D is uncacheable: Add check that D still links to C
> +    // - C is uncacheable: Modifying C.__proto__ will still reshape B

Maybe: will still reshape B if B is not uncacheable

That makes the "B is uncacheable" case below a bit clearer.

@@ +590,5 @@
> +    // Let's consider the effect of UNCACHEABLE_PROTO flag on our example:
> +    // - D is uncacheable: Add check that D still links to C
> +    // - C is uncacheable: Modifying C.__proto__ will still reshape B
> +    // - B is uncacheable: Add shape check C since B will not reshape OR check
> +    //                     proto of B and C

check proto of D and C (not B and C)?

@@ +622,5 @@
> +
> +    // Synchronize pobj and protoId.
> +    MOZ_ASSERT(pobj == obj || pobj == obj->staticPrototype());
> +    ObjOperandId protoId = (pobj == obj) ? objId
> +                                         : writer.loadProto(objId);

If pobj == holder, this will emit a dead LoadProto; please add a |pobj == holder| check.

::: js/src/vm/JSObject.cpp
@@ +2005,5 @@
> +    // invalidation, we apply heuristics to decide when to apply this and when
> +    // to require a guard.
> +    //
> +    // Heuristics:
> +    //  - Always reshape singleton objects to avoid hurting iterator cache.

We used to check hasUncacheableProto in CanCacheIterableObject, see ESR52: https://dxr.mozilla.org/comm-esr52/rev/af6372f41c16e34710c78d5434943651faba2b14/mozilla/js/src/jsiter.cpp#802

However, since bug 1375505, the iterator cache supports uncacheable-proto objects, so this line is no longer true.

::: js/src/vm/ObjectGroup.cpp
@@ +540,5 @@
> +
> +            // |ReshapeForProtoMutation| ensures singletons will reshape when
> +            // prototype is mutated so clear the UNCACHEABLE_PROTO flag.
> +            if (protoObj->hasUncacheableProto()) {
> +                RootedNativeObject nobj(cx, &protoObj->as<NativeObject>());

Nit: HandleNativeObject nobj = protoObj.as<NativeObject>();

Comment 11

[Ted Campbell [:tcampbell]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> check proto of D and C (not B and C)?
Yep, good catch.


> If pobj == holder, this will emit a dead LoadProto; please add a |pobj ==
> holder| check.
I had considered the cacheable holder case (caught by the early exit above), but not the uncacheable holder case. Good find.


> However, since bug 1375505, the iterator cache supports uncacheable-proto
> objects, so this line is no longer true.
Oh.. I knew this.. Good point. I'll fix comment to make not that this isn't true, but will leave behavior and open a follow-up bug for someone to experiment.

Comment 12

[Ted Campbell [:tcampbell]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> It is possible to make this private, to force everyone to use guardGroupFor*
> below?

I went back-and-forth on this, but I think you are right that we should be more forceful about this. I'll change |SetPropIRGenerator::tryAttachAddSlotStub| to use guardGroupForTypeBarrier and keep the assertions and comments. The assertion should help avoid mistakes if behavior of guardGroupForTypeBarrier were to ever change.

Comment 13

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8951287 [details] [diff] [review]
0002r4-Bug-1438086-Cleanup-shape-teleporting-optimization.patch

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

Some more comment nits I noticed while reading the comment via irc; wanted to wait until after the review. Very nice comment overall :)

::: js/src/jit/CacheIR.cpp
@@ +548,5 @@
> +    // ensure |holder| is still on the prototype chain of |obj| and we haven't
> +    // introduced any covering definitions.
> +    //
> +    // For each item in the proto chain before holder, we must ensure that
> +    // [[GetPrototypeOf]] still has expected result, and that

missing "the" before expected?

@@ +565,5 @@
> +    //      D  ->  C  ->  B{x: 3}  ->  A  -> null
> +    //
> +    // When accessing |D.x| we refer to D as the "receiver", and B as the
> +    // "holder". To optimize this access we need to ensure that neither D nor C
> +    // has since defined a covering property 'x'. Since C is prototype that we

missing "a" or "the" before prototype.

@@ +578,5 @@
> +    // The second condition we must verify is that the prototype chain was not
> +    // mutated. The same mechanism as above is used. When the prototype link is
> +    // changed, we generate a new shape for the object. If the object who's
> +    // link we are mutating is itself a prototype, we regenerate shapes down
> +    // the chain. The means the same two shape checks as above are sufficient.

s/The/This/ (or That)

@@ +582,5 @@
> +    // the chain. The means the same two shape checks as above are sufficient.
> +    //
> +    // Unfortunately we don't stop there and add further caveats. We may set
> +    // the UNCACHEABLE_PROTO flag on the shape of an object to indicate that it
> +    // will not generate a new shape if it's prototype link is modified. If the

s/it's/its/

@@ +586,5 @@
> +    // will not generate a new shape if it's prototype link is modified. If the
> +    // object is itself a prototype we follow the shape chain and regenerate
> +    // shapes (if they aren't themselves uncacheable).
> +    //
> +    // Let's consider the effect of UNCACHEABLE_PROTO flag on our example:

missing "the" before UNCACHEABLE_PROTO?

@@ +601,4 @@
>      MOZ_ASSERT(obj != holder);
>  
> +    // Only DELEGATE objects participate in teleporting so peel off the first
> +    // object in chain if needed and handle it directly.

missing "the" before chain?

@@ +611,5 @@
> +    }
> +    MOZ_ASSERT(pobj->isDelegate());
> +
> +    // In the common case, holder has a cacheable prototype and will regenerate
> +    // its shape if any (delegate) objects in proto chain are updated.

missing "the" before proto?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1501ec7b3b37
Cleanup IC group guards. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a90bcec86dac
Cleanup shape teleporting optimization. r=jandem

Comment 15

[Ted Campbell [:tcampbell]]

Nits addressed and landed.

Comment 16

[Ted Campbell [:tcampbell]]

TODO: Add guardShapeFor* helpers

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1501ec7b3b37
https://hg.mozilla.org/mozilla-central/rev/a90bcec86dac

Comment 18

[Ted Campbell [:tcampbell]]

Attachment: 0003-Bug-1438086-Cleanup-TestMatchingReceiver-in-CacheIR.patch

This adds guardShapeForClass / guardShapeForOwnProperties and then abstracts things behind TestMatching*Receiver helpers.

Asserts are a little different, but generated JIT should be identical. The one exception is an unnecessary guardClass was removed from tryAttachArrayPush.

Comment 19

[Ted Campbell [:tcampbell]]

Attachment: 0004-Bug-1438086-Add-GeneratePrototypeGuardsForMissing-to.patch

Cleanup code to generate prototype guards for the missing property case. This should generate identical JIT code.

Comment 20

[Ted Campbell [:tcampbell]]

Comment on attachment 8953313 [details] [diff] [review]
0004-Bug-1438086-Add-GeneratePrototypeGuardsForMissing-to.patch

This patch actually did introduce some guards. Revisiting..

Comment 21

[Ted Campbell [:tcampbell]]

Attachment: 0005-Bug-1438086-Add-TestMatchingHolder-to-CacheIR.patch

This refactors a bunch of bare shape checks to use TestMatchingHolder.

Comment 22

[Ted Campbell [:tcampbell]]

Part 4 turned out to be a mess and I am revisiting my approach. Parts 3/5 are both good to go.

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 8953312 [details] [diff] [review]
0003-Bug-1438086-Cleanup-TestMatchingReceiver-in-CacheIR.patch

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

Sweet. I wanted to make similar changes for Spectre object type mitigations: we need to mitigate guardShapeForOwnProperties and guardShapeForClass shape guards, but shape guards for proto objects that are just there for correctness (not security) we don't really care about I think. So I like where this is going.

::: js/src/jit/CacheIR.cpp
@@ +600,5 @@
> +    } else if (obj->is<TypedObject>()) {
> +        MOZ_ASSERT(!obj->group()->hasUncacheableProto());
> +    } else if (obj->is<ProxyObject>()) {
> +        MOZ_ASSERT(!obj->hasUncacheableProto());
> +    }

Nit: no {}. Also these asserts are implied by the obj->is<NativeObject>() assertion above (but I don't mind keeping them for documentation reasons).

::: js/src/jit/CacheIR.h
@@ +549,5 @@
>      }
> +    void guardShapeForClass(ObjOperandId obj, Shape* shape) {
> +        // Guard shape to ensure that object class is unchanged. This is true
> +        // for all shapes.
> +        MOZ_ASSERT(shape);

Hm can we move this assert to guardShape?

@@ +555,5 @@
> +    }
> +    void guardShapeForOwnProperties(ObjOperandId obj, Shape* shape) {
> +        // Guard shape to detect changes to (non-dense) own properties. This
> +        // also implies |guardShapeForClass|.
> +        MOZ_ASSERT(shape);

MOZ_ASSERT(shape->getObjectClass()->isNative()); ?

Comment 24

[Ted Campbell [:tcampbell]]

(In reply to Jan de Mooij [:jandem] from comment #23)
> ::: js/src/jit/CacheIR.cpp
> @@ +600,5 @@
> > +    } else if (obj->is<TypedObject>()) {
> > +        MOZ_ASSERT(!obj->group()->hasUncacheableProto());
> > +    } else if (obj->is<ProxyObject>()) {
> > +        MOZ_ASSERT(!obj->hasUncacheableProto());
> > +    }
> 
> Nit: no {}. Also these asserts are implied by the obj->is<NativeObject>()
> assertion above (but I don't mind keeping them for documentation reasons).

These are for documentation purposes and to detect if we ever need more guards in the future.


> MOZ_ASSERT(shape->getObjectClass()->isNative()); ?

Great idea!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e379efa4ff54
Cleanup TestMatchingReceiver in CacheIR. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8a49c03cad
Add TestMatchingHolder to CacheIR. r=jandem

Comment 26

[Ted Campbell [:tcampbell]]

Attachment: 0006-Bug-1438086-Use-ShapeGuardProtoChain-for-EmitReadSlo.patch

Part 3 introduced a null-deref regression when someone swapped the protochain with null. This uses ShapeGuardProtoChain instead of the manual loop.

NOTE: Eventaully this will be combined with GeneratePrototypeHoleGuards.

Comment 27

[Ted Campbell [:tcampbell]]

(In reply to Ted Campbell [:tcampbell] from comment #26)
> Created attachment 8953565 [details] [diff] [review]
> 0006-Bug-1438086-Use-ShapeGuardProtoChain-for-EmitReadSlo.patch

I also moved ShapeGuardProtoChain earlier so it is defined. The code is unchanged.

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8953565 [details] [diff] [review]
0006-Bug-1438086-Use-ShapeGuardProtoChain-for-EmitReadSlo.patch

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

Another proto walk gone.

Comment 29

[Ted Campbell [:tcampbell]]

(In reply to Ted Campbell [:tcampbell] from comment #26)
> Created attachment 8953565 [details] [diff] [review]
> 0006-Bug-1438086-Use-ShapeGuardProtoChain-for-EmitReadSlo.patch
> 
> Part 3 introduced a null-deref regression when someone swapped the
> protochain with null.

Oops.. Part 2 introduced the regression. Landed earlier in the week.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a5245ff819
Use ShapeGuardProtoChain for EmitReadSlotGuard. r=jandem

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e379efa4ff54
https://hg.mozilla.org/mozilla-central/rev/2d8a49c03cad
https://hg.mozilla.org/mozilla-central/rev/a1a5245ff819