CacheIR: Add SetProp/SetElem WindowProxy stubs

RESOLVED FIXED in Firefox 55

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf-])

Attachments

(1 attachment)

We have these for GetProp/GetElem now but we should also optimize slots and setters. This happens when code does |window.foo = bar| or |this.x = y| in the browser, so it's likely pretty common.
bz, just curious, are there any interesting native setters in the browser where this could help? (Don't spend any time looking into this, we should optimize setters anyway because it's pretty simple and we want it for scripted setters.)

My #1 goal atm is replacing the Ion SetProp/SetElem code, but I should be able to get to this once that's done.
Priority: -- → P3
Optimizing slot sets here makes a lot of sets.

For native setters..  I guess there's the event handlers, if people keep resetting them (which is unlikely).  Looking quickly at non-readonly IDL attributes on Window, there's nothing else there that's in any way performance-sensitive.
> Optimizing slot sets here makes a lot of sets.

A lot of _sense_.
Whiteboard: [qf-]
Posted patch PatchSplinter Review
This handles simple slot sets. A micro-benchmark doing |this.x = i;| 10000000 times improves from 2213 ms to 35 ms. I get 7 ms in Chrome, to get there we need to inline this in IonBuilder. Apparently this is something JSC doesn't optimize very well because Safari gets 780 ms.

I added some logging and we attach this a few times on some websites like Google Docs. I also checked the setter case but I don't see as many hits for that.

The group guard we do may be unnecessary because we also guard on the shape and there's a single global per compartment, but the overhead is small so I decided to keep it.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8850884 - Flags: review?(evilpies)
Comment on attachment 8850884 [details] [diff] [review]
Patch

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

Nice! Extending IonBuild/BaslineInspector to support this should be too difficult if we wanted to do that.

::: js/src/jit/CacheIR.cpp
@@ +2962,5 @@
> +    // Now try to do the set on the Window (the current global).
> +    Handle<GlobalObject*> windowObj = cx_->global();
> +
> +    RootedShape propShape(cx_);
> +    if (CanAttachNativeSetSlot(cx_, windowObj, id, isTemporarilyUnoptimizable_, &propShape)) {

Invert this condition.

@@ +2973,5 @@
> +        writer.guardGroup(windowObjId, windowObj->group());
> +        typeCheckInfo_.set(windowObj->group(), id);
> +
> +        EmitStoreSlotAndReturn(writer, windowObjId, windowObj, propShape, rhsId);
> +        trackAttached("WindowProxySlot");

Super-Nit: I usually put a blank line before trackAttached.
Attachment #8850884 - Flags: review?(evilpies) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0e044db9e1
Add IC support for setting existing properties on WindowProxies. r=evilpie
https://hg.mozilla.org/mozilla-central/rev/ab0e044db9e1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Duplicate of this bug: 1009447
You need to log in before you can comment on or make changes to this bug.