1157231 - Optimize calls to own property setters

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Baseline and IonBuilder currently optimize setters on the prototype but not own setters. The attached patch fixes this and also makes the code look a bit more like the getter case.

For a micro-benchmark I'll attach shortly I get (before/after):

protosetter  7  /  7
ownsetter   64  / 12

The ownsetter case is still a bit slower in Ion due to a silly bug; patch for that coming up too.

I fixed this for getters in bug 1128646, but that patch landed in the previous release cycle so I'm filing a new bug to make it easier to track this.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Setter microbenchmark

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> The ownsetter case is still a bit slower in Ion due to a silly bug; patch
> for that coming up too.

Filed bug 1157239.

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8595966 [details] [diff] [review]
Patch

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

Good. r=me

::: js/src/jit/BaselineIC.cpp
@@ +6265,5 @@
> +                if (isOwnSetter)
> +                    setPropStub->receiverGuard().update(receiverGuard);
> +
> +                MOZ_ASSERT(setPropStub->holderShape() != holder->lastProperty() ||
> +                           !setPropStub->receiverGuard().matches(receiverGuard),

It's safe to update this before the assert because holderShape() and receiverGuard() will yield the same thing initially, so we can still assert we failed the shape guard based on the holder in the isOwnSetter case, right?

@@ +6274,3 @@
>                  setPropStub->holderShape() = holder->lastProperty();
> +
> +                // Make sure to update the getter, since a shape change might

nit: copy/paste error: this comment refers to getters.

::: js/src/jit/BaselineIC.h
@@ +5367,5 @@
>          uint32_t pcOffset_;
>  
>          virtual int32_t getKey() const {
>              return static_cast<int32_t>(kind) |
> +                   (ReceiverGuard::keyBits(receiver_) << 16) |

Not necessary, since the problem is pervasive, but is it worth adding a static assert that keyBits agrees to only be 3 bits wide? Every time I see one of these key calculations, I get sweaty palms about bit collisions.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/723039c4f514

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Eric Faust [:efaust] from comment #3)
> Not necessary, since the problem is pervasive, but is it worth adding a
> static assert that keyBits agrees to only be 3 bits wide? Every time I see
> one of these key calculations, I get sweaty palms about bit collisions.

Yeah, I think it'd be nice to do something more structured/safe eventually. Maybe some template magic so you can pass in the width and shift amount, Key<0, 2>(foo) | Key<2, 8>(bar).

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/723039c4f514