For all the cases other than property addition, generating baseline ICs after the operation is a bit silly: all the information needed to generate the IC is present before the operation. Right now we do this thing where we first do the operation, then generate the IC, but not if the operation mutated the object's shape (see bug 1084150). But while the add-property IC needs both before and after information, I think all the other ICs only need before. So we should strongly consider doing those before doing the actual operation. Or something along those lines.

A local hack to try a fix for this bug shows us ending up 2x faster here, fwiw. And about 30x faster on the microbenchmark.

Filed bug 1094491 on getters and will focus on setters here.

The property addition stub needs to know both the before and after state to detect the property add, and as far as I can tell the slot write stub wants to know the TI info for after the set, but the accessor stubs just need to know the before state. For now we're just splitting up the two codepaths with no other behavior changes.

This way we don't end up guarding on the post-set shape, which may differ from the pre-set shape and thus always fail the guard.

Comment on attachment 8517779 [details] [diff] [review] part 1. Split apart the attachment of property set IC stubs into separate functions for value properties and accessor properties in Baseline Review of attachment 8517779 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineIC.cpp @@ +7722,5 @@ > // > // SetProp_Fallback > // > > +// Attach an optimized property set stub for a SETPROP/SETGNAME/SETNAME op on a value property. Nit: comments should fit within 80 columns. @@ +7809,5 @@ > > + return true; > +} > + > +// Attach an optimized property set stub for a SETPROP/SETGNAME/SETNAME op on an accessor property. Same here.

Comment on attachment 8517780 [details] [diff] [review] part 2. Move TryAttachSetPropStub to before we actually perform the set Review of attachment 8517780 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/jit/BaselineIC.cpp @@ +7926,5 @@ > + // failed to attach a stub is one of those temporary reasons, since we might > + // end up attaching a stub for the exact same access later. > + bool isTemporarilyUnoptimizable = false; > + // Check if debug mode toggling made the stub invalid. > + if (!stub.invalid() && Nit: we can remove this check. We only need it after we do something that can end up running arbitrary JS (and end up doing scary Debugger things), but this code runs *before* we call setters etc so it's not an issue. @@ +7928,5 @@ > + bool isTemporarilyUnoptimizable = false; > + // Check if debug mode toggling made the stub invalid. > + if (!stub.invalid() && > + // TODO: Discard all stubs in this IC and replace with generic setprop > + // stub if we hit the limit. Pre-existing nit: please remove this comment and the similar one below (and drop the {} there); it doesn't really add anything. @@ +7933,5 @@ > + stub->numOptimizedStubs() < ICSetProp_Fallback::MAX_OPTIMIZED_STUBS && > + !TryAttachSetAccessorPropStub(cx, script, pc, stub, obj, oldShape, oldType, oldSlots, > + name, id, rhs, &attached, &isTemporarilyUnoptimizable)) > + { > + return false; Nit: some trailing whitespace on this line

Comment on attachment 8517782 [details] [diff] [review] part 4. Strengthen the asserts in UpdateExistingSetPropCallStubs now that the stronger asserts should hold Review of attachment 8517782 [details] [diff] [review]: ----------------------------------------------------------------- Perfect.

> Pre-existing nit: please remove this comment and the similar one below ... > it doesn't really add anything. Is doing the discarding thing not a goal?

(In reply to Boris Zbarsky [:bz] from comment #11) > > Pre-existing nit: please remove this comment and the similar one below ... > > it doesn't really add anything. > > Is doing the discarding thing not a goal? Thinking about it more, let's keep the original comment. It could be more efficient to discard stubs in polymorphic cases so we should probably do that at some point... I think we can remove the new comment though (the one before we perform the SetProp operation), because if we discard stubs somewhere we'd probably do it after the SETPROP operation, but if you prefer to have the comment there too I'm fine with it either way.

Only having the comment after the set makes sense to me. Thanks!

https://hg.mozilla.org/mozilla-central/rev/48f09d992105 https://hg.mozilla.org/mozilla-central/rev/f4dce09a7ac9 https://hg.mozilla.org/mozilla-central/rev/f8939fe12c12 https://hg.mozilla.org/mozilla-central/rev/7bf195ca830a