1340496 - CacheIR: Add SetProp/SetElem WindowProxy stubs

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Optimizing slot sets here makes a lot of sets.

A lot of _sense_.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 5

[Tom Schuster [:evilpie]]

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0e044db9e1
Add IC support for setting existing properties on WindowProxies. r=evilpie

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ab0e044db9e1