Add SetProp/SetElem stub for (DOM) Proxies

RESOLVED FIXED in Firefox 54

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: evilpie, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(3 attachments)

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.
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
Given comment 1 marking this P1.
Priority: -- → P1
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)
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)
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 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 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 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+
Blocks: 1341067
(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.
(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.
(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.
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
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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.