Closed Bug 1340496 Opened 6 years ago Closed 6 years ago

CacheIR: Add SetProp/SetElem WindowProxy stubs


(Core :: JavaScript Engine: JIT, defect, P3)




Performance Impact none
Tracking Status
firefox55 --- fixed


(Reporter: jandem, Assigned: jandem)


(Blocks 1 open bug)



(1 file)

We have these for GetProp/GetElem now but we should also optimize slots and setters. This happens when code does | = 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-]
Attached 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
Attachment #8850884 - Flags: review?(evilpies)
Comment on attachment 8850884 [details] [diff] [review]

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
Add IC support for setting existing properties on WindowProxies. r=evilpie
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Performance Impact: --- → -
Whiteboard: [qf-]
You need to log in before you can comment on or make changes to this bug.