Optimize calls to own property setters

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments)

Posted patch PatchSplinter Review
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.
Attachment #8595966 - Flags: review?(efaustbmo)
Depends on: 1157239
(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 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.
Attachment #8595966 - Flags: review?(efaustbmo) → review+
(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).
https://hg.mozilla.org/mozilla-central/rev/723039c4f514
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1247877
You need to log in before you can comment on or make changes to this bug.