1089050 - Generate baseline set ICs for accessor properties before performing the relevant operation

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: A super-micro-benchmark showing a worst-case scenario for this bug

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: A not-quite-as-micro benchmark for the same thing

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Split apart the attachment of property set IC stubs into separate functions for value properties and accessor properties in Baseline

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Move TryAttachSetPropStub to before we actually perform the set

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 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove now-unnecesary old* arguments to TryAttachSetAccessorPropStub and its callees

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Strengthen the asserts in UpdateExistingSetPropCallStubs now that the stronger asserts should hold

Comment 8

[Jan de Mooij [:jandem]]

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 9

[Jan de Mooij [:jandem]]

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 10

[Jan de Mooij [:jandem]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 12

[Jan de Mooij [:jandem]]

(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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Carsten Book [:Tomcat]]

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