Closed Bug 1438086 Opened 2 years ago Closed 2 years ago

Shape teleporting spring cleaning

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

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.
Assignee: nobody → tcampbell
Priority: -- → P2
Refactor CacheIRWriter::guardGroup calls to add more assertions and be clearer about what the group guard is achieving.
This patch tries to cleanup shape teleporting and make it easier to reason about.
Fix missing error propagation.
Attachment #8950837 - Attachment is obsolete: true
Only use GeneratePrototypeGuards when holder exists. The requirements for guarding a missing property are very different.
Attachment #8950936 - Attachment is obsolete: true
Stop using GeneratePrototypeGuards when holder is null. Use toDictionary before clearing a shape flag. Rebase over file renames.
Attachment #8951131 - Attachment is obsolete: true
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.
Attachment #8950836 - Flags: review?(jdemooij)
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.
Attachment #8951287 - Flags: review?(jdemooij)
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
Attachment #8950836 - Flags: review?(jdemooij) → review+
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>();
Attachment #8951287 - Flags: review?(jdemooij) → review+
(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.
(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 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?
Nits addressed and landed.
TODO: Add guardShapeFor* helpers
Keywords: leave-open
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.
Attachment #8953312 - Flags: review?(jdemooij)
Cleanup code to generate prototype guards for the missing property case. This should generate identical JIT code.
Attachment #8953313 - Flags: review?(jdemooij)
Comment on attachment 8953313 [details] [diff] [review]
0004-Bug-1438086-Add-GeneratePrototypeGuardsForMissing-to.patch

This patch actually did introduce some guards. Revisiting..
Attachment #8953313 - Attachment is obsolete: true
Attachment #8953313 - Flags: review?(jdemooij)
This refactors a bunch of bare shape checks to use TestMatchingHolder.
Attachment #8953337 - Flags: review?(jdemooij)
Part 4 turned out to be a mess and I am revisiting my approach. Parts 3/5 are both good to go.
Depends on: 1440554
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()); ?
Attachment #8953312 - Flags: review?(jdemooij) → review+
Attachment #8953337 - Flags: review?(jdemooij) → review+
(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!
Pushed by tcampbell@mozilla.com:
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
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.
Attachment #8953565 - Flags: review?(jdemooij)
(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 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.
Attachment #8953565 - Flags: review?(jdemooij) → review+
(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.
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a5245ff819
Use ShapeGuardProtoChain for EmitReadSlotGuard. r=jandem
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1459754
Depends on: 1507433
Target Milestone: --- → mozilla60
See Also: → 1539467
You need to log in before you can comment on or make changes to this bug.