Closed
Bug 1340496
Opened 6 years ago
Closed 6 years ago
CacheIR: Add SetProp/SetElem WindowProxy stubs
Categories
(Core :: JavaScript Engine: JIT, defect, P3)
Core
JavaScript Engine: JIT
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
4.46 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
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
![]() |
||
Comment 2•6 years ago
|
||
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•6 years ago
|
||
> Optimizing slot sets here makes a lot of sets.
A lot of _sense_.
Updated•6 years ago
|
Whiteboard: [qf-]
Assignee | ||
Comment 4•6 years ago
|
||
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•6 years ago
|
||
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
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ab0e044db9e1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•11 months ago
|
Performance Impact: --- → -
Whiteboard: [qf-]
You need to log in
before you can comment on or make changes to this bug.
Description
•