Closed
Bug 1338828
Opened 7 years ago
Closed 7 years ago
Add SetProp/SetElem stub for (DOM) Proxies
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: evilpie, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
15.15 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
2.78 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
9.16 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
Ion already has SetProp/SetElem stubs for DOM proxies and an additional generic proxy stub. Porting this to CacheIR will improve Baseline and won't regress Ion after switching to CacheIR.
Assignee | ||
Comment 1•7 years ago
|
||
After bug 1337024 this will be the last thing blocking conversion of the Ion SetProp/SetElem code, so I'll probably start working on this today or tomorrow. Having these proxy stubs in Baseline is really nice too of course.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•7 years ago
|
||
This adds generic stubs for proxies. After this we only need to add the specialized (faster) stubs for DOM proxies. The code for this is based on the GetProp/GetElem code, the main difference is that we now have the RHS Value and a "bool strict" argument. A bit annoying is that we don't have enough registers in CallProxySetByValue to get a scratch register on x86, so I fixed that by storing the object to the frame's scratch slot and restoring it after we enter the stub frame. evilpie wrote a lot of the GetProp proxy code so I'd ask him to review, but he's busy these weeks.
Attachment #8838525 -
Flags: review?(hv1989)
Assignee | ||
Comment 4•7 years ago
|
||
The shadowed case is easy, just use the CallProxySet instruction added by the previous patch. Note that DOMExpando is a subset of DOMShadowed, so we can use the same code for that. The difference is that DOMExpando can be optimized more to be much faster, that's bug 1133423.
Attachment #8838539 -
Flags: review?(hv1989)
Assignee | ||
Comment 5•7 years ago
|
||
This is used to call a setter on the prototype of a DOM proxy, when we know the proxy doesn't shadow the property. If we fail to do this we use the generic proxy code. Again, this is very similar to GetPropIRGenerator::tryAttachDOMProxyUnshadowed, except we only care about setters here (because you can't do a "set slot" on an object on the prototype). That's all for this bug, I'll optimize the DOMExpando case later in bug 1133423.
Attachment #8838660 -
Flags: review?(hv1989)
Comment 6•7 years ago
|
||
Comment on attachment 8838525 [details] [diff] [review] Part 1 - Add generic stubs Review of attachment 8838525 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Shouldn't we use "Result<>" for tryAttach? ::: js/src/jit/BaselineCacheIRCompiler.cpp @@ +1561,5 @@ > + AutoStubFrame stubFrame(*this); > + stubFrame.enter(masm, obj); > + > + masm.loadPtr(Address(BaselineFrameReg, 0), obj); > + masm.loadPtr(Address(obj, scratchOffset), obj); Can't we do? masm.loadPtr(Address(BaselineFrameReg, scratchOffset), obj);
Attachment #8838525 -
Flags: review?(hv1989) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8838539 [details] [diff] [review] Part 2 - Shadowed DOM proxies Review of attachment 8838539 [details] [diff] [review]: ----------------------------------------------------------------- That was easy! (IIUC the difference is only the shape check instead of the class check, right? Which should be a little bit faster) ::: js/src/jit/CacheIR.cpp @@ +2543,5 @@ > + ValOperandId rhsId) > +{ > + MOZ_ASSERT(IsCacheableDOMProxy(obj)); > + > + maybeEmitIdGuard(id); Can we have SetElem here? If not: please assert If we can: should we use the "value" variant? Just like the generic stub?
Attachment #8838539 -
Flags: review?(hv1989) → review+
Comment 8•7 years ago
|
||
Comment on attachment 8838660 [details] [diff] [review] Part 3 - Unshadowed DOM proxies Review of attachment 8838660 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Thanks!
Attachment #8838660 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #6) > Looks good. Shouldn't we use "Result<>" for tryAttach? The return bool means "attached" or "not attached". > > + masm.loadPtr(Address(BaselineFrameReg, 0), obj); > > + masm.loadPtr(Address(obj, scratchOffset), obj); > > Can't we do? > > masm.loadPtr(Address(BaselineFrameReg, scratchOffset), obj); We entered the stub frame, it changes the frame pointer, so we first have to load the original frame pointer. I'll add a comment.
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #7) > If we can: should we use the "value" variant? Just like the generic stub? Good idea. I'll post a new patch to change this for GetElem too.
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #10) > > If we can: should we use the "value" variant? Just like the generic stub? > > Good idea. I'll post a new patch to change this for GetElem too. Actually I think it makes sense to keep this for now and fix the generic proxy stub later as described in bug 1328140 comment 3 paragraph 2. Basically that would let us specialize for a particular jsid and when we attach too many of these stubs, we'll discard them and try again with generic by-value proxy stubs.
Comment 12•7 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/23120d7bc4d6 part 1 - Add CacheIR SetProp/SetElem stubs for proxies. r=h4writer https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7c7758fda8 part 2 - Add CacheIR SetProp/SetElem stubs for shadowing DOM proxies. r=h4writer https://hg.mozilla.org/integration/mozilla-inbound/rev/eb661732bcde part 3 - Add CacheIR SetProp/SetElem stubs for unshadowed setter calls on DOM proxies. r=h4writer
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/23120d7bc4d6 https://hg.mozilla.org/mozilla-central/rev/8f7c7758fda8 https://hg.mozilla.org/mozilla-central/rev/eb661732bcde
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•