1319437 - Port Baseline GETPROP DOM proxy stubs to CacheIR

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

These are the last GETPROP stubs that work on objects.

My patch stack removes a lot of boilerplate:

 14 files changed, 582 insertions(+), 849 deletions(-)

And it handles more cases than before, because it's written like the Ion ICs. For instance, unlike the current Baseline ICs, we will now handle lookups on generic (non-DOM) proxies (by doing a callVM to Proxy::get), and we can optimize non-getter properties on the prototype (like document.getElementById).

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add a generic proxy stub

This patch adds a stub that does a callVM to Proxy::get for all proxies.

I had to add support for storing jsid in CacheIR stubs, and I also had to add VMFunction support for jsid/HandleId arguments (weird that we don't need that anywhere else).

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add a generic proxy stub

Sorry, forgot to qref.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Optimize shadowing DOM proxies

This ports the Ion IC code for this to CacheIR and removes the Baseline stub. This patch is mostly code removal.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Support 64-bit stub fields

Currently all CacheIR stub fields are word-sized, but the next patch needs to store an uint64 value (ExpandoAndGeneration::generation) on all platforms. We will also need this for JS::Value at some point so I added support for that as well.

A bit annoying to rewrite code to handle both cases, but not too bad.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Unshadowed case

This ports tryAttachDOMProxyUnshadowed from the Ion IC to Cache IR, and removes the Baseline stubs that handled some of these cases.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Use CustomAutoRooter for CacheIRWriter

CacheIRWriter currently has an AutoCheckCannotGC, because we don't trace the stub fields in it. The previous patch adds a function call (DOMProxyShadows) that can GC, so we need to fix this.

I made CacheIRWriter a JS::CustomAutoRooter and added a MOZ_RELEASE_ASSERT to ensure there are no stub fields when we GC. If this ever fails we can add proper tracing for it but I hope we don't have to.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8813276 [details] [diff] [review]
Part 5 - Use CustomAutoRooter for CacheIRWriter

Review of attachment 8813276 [details] [diff] [review]:
-----------------------------------------------------------------

While this does work, we'd like to get rid of CustomAutoRooter.  Can you use Rooted for this instead?  You should be able to do Rooted<CacheIRWriter> and it will magically call your trace() method.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #7)
> While this does work, we'd like to get rid of CustomAutoRooter.  Can you use
> Rooted for this instead?

Yeah, sure. What I liked about the AutoRooter is that it magically works everywhere, but I suppose I can mark CacheIRWriter as JS_HAZ_GC_POINTER to make the static analysis know about the GC pointers...

Comment 9

[Hannes Verschore [:h4writer]]

Marking P1 since we have the patch that is close to finish and I think Jan wants to land this in this release. Feel free to put P2 if needed. But we still have till end of Januari.

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8813254 [details] [diff] [review]
Part 1 - Add a generic proxy stub

Review of attachment 8813254 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineCacheIR.cpp
@@ +1224,5 @@
> +    // We use ICTailCallReg when entering the stub frame, so ensure it's not
> +    // used for something else.
> +    Maybe<AutoScratchRegister> tail;
> +    if (allocator.isAllocatable(ICTailCallReg))
> +        tail.emplace(allocator, masm, ICTailCallReg);

I do not understand why this is conditional.

If it is not allocatable, this could mean that the obj value could potentially be located in the ICTailCallReg, and that it would be overwritten by the masm.Pop(ICTailCallReg) done under ensureStubFrame.

By making it unconditional, it should be allocated by spilling its current content.

::: js/src/jit/CacheIR.cpp
@@ +370,5 @@
> +    MOZ_ASSERT(obj->is<ProxyObject>());
> +
> +    emitted_ = true;
> +
> +    writer.guardIsProxy(objId);

maybe-comment:

This does not need to check if the id is the same when the stub is executed, because this would generate a stub for a constant name_ property [1], taken from the bytecode of the function.

[1] http://searchfox.org/mozilla-central/source/js/src/jit/SharedIC.cpp#2677,2697

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8813254 [details] [diff] [review]
Part 1 - Add a generic proxy stub

Review of attachment 8813254 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineCacheIR.cpp
@@ +1224,5 @@
> +    // We use ICTailCallReg when entering the stub frame, so ensure it's not
> +    // used for something else.
> +    Maybe<AutoScratchRegister> tail;
> +    if (allocator.isAllocatable(ICTailCallReg))
> +        tail.emplace(allocator, masm, ICTailCallReg);

allocator.isAllocatable()  looks in the set of allocatable registers on the architecture (ARM vs others), which is a constant, and initialized with GeneralRegisterSet::All().

So this was my miss-understanding.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Use CustomAutoRooter for CacheIRWriter

As discussed on IRC, Rooted<> makes it harder to call methods on it without adding boilerplate so I went wit CustomAutoRooter for now.

I also removed the Maybe<> we used and moved the CacheIRWriter into the generator class so we don't need to pass it to each member function.

Comment 13

[Jon Coppeard (:jonco)]

Comment on attachment 8814369 [details] [diff] [review]
Part 5 - Use CustomAutoRooter for CacheIRWriter

Review of attachment 8814369 [details] [diff] [review]:
-----------------------------------------------------------------

The cleanups looks good.

::: js/src/jit/CacheIR.h
@@ +433,5 @@
>      void callProxyGetResult(ObjOperandId obj, jsid id) {
>          writeOpWithOperandId(CacheOp::CallProxyGetResult, obj);
>          addStubField(uintptr_t(JSID_BITS(id)), StubField::Type::Id);
>      }
> +} JS_HAZ_GC_POINTER;

I'm unsure whether the analysis will think that use of this class over a possible GC is a hazard.  On the one hand it has this annotation (which is a hazard if it's not rooted) and on the other hand it is a CustomAutoRooter (which are ignored).  If you haven't done a hazard analysis try run yet I'd suggest you do one.

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8813259 [details] [diff] [review]
Part 2 - Optimize shadowing DOM proxies

Review of attachment 8813259 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CacheIR.cpp
@@ +409,5 @@
>  
> +    // Skim off DOM proxies.
> +    if (IsCacheableDOMProxy(obj)) {
> +        RootedId id(cx_, NameToId(name_));
> +        DOMProxyShadowsResult shadows = GetDOMProxyShadowsCheck()(cx_, obj, id);

This is calling a function provided by the embedders of SpiderMonkey, but the signature of this function does not enforce the JS::AutoCheckCannotGC instantiated in the caller of this function (GetPropIRGenerator::tryAttachStub).

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8813267 [details] [diff] [review]
Part 3 - Support 64-bit stub fields

Review of attachment 8813267 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineCacheIR.cpp
@@ +1506,5 @@
> +          case StubField::Type::RawInt64:
> +            *reinterpret_cast<uint64_t*>(destWords) = stubFields_[i].asInt64();
> +            break;
> +          case StubField::Type::Value:
> +            InitGCPtr<JS::Value>(destWords, stubFields_[i].asInt64());

follow-up:  These stub values are used for tracing by the JIT, and read by the generated cacheIR code, so maybe it would make sense to align them on 64 bits as well.

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8813259 [details] [diff] [review]
Part 2 - Optimize shadowing DOM proxies

Review of attachment 8813259 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CacheIR.cpp
@@ +409,5 @@
>  
> +    // Skim off DOM proxies.
> +    if (IsCacheableDOMProxy(obj)) {
> +        RootedId id(cx_, NameToId(name_));
> +        DOMProxyShadowsResult shadows = GetDOMProxyShadowsCheck()(cx_, obj, id);

Jan commented on IRC, that part 5 is removing the AutoCheckCannotGC, so there is no more blocker from my point of view.

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8813269 [details] [diff] [review]
Part 4 - Unshadowed case

Review of attachment 8813269 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CacheIR.cpp
@@ +257,5 @@
> +        writer.callNativeGetterResult(objId, target);
> +        return;
> +    }
> +
> +    MOZ_ASSERT(IsCacheableGetPropCallScripted(obj, holder, shape));

What about IsCacheableGetPropCallPropertyOp which is handled in [1]?


[1] http://searchfox.org/mozilla-central/source/js/src/jit/IonCaches.cpp#980

@@ +441,5 @@
> +}
> +
> +bool
> +GetPropIRGenerator::tryAttachDOMProxyUnshadowed(CacheIRWriter& writer, HandleObject obj,
> +                                                ObjOperandId objId, bool resetNeeded)

nit: resetNeeded is unused.

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #17)
> What about IsCacheableGetPropCallPropertyOp which is handled in [1]?

Good point. Baseline currently doesn't handle PropertyOp getters at all, they're kind of deprecated. I'll file a follow-up bug to add it to CacheIR, we will need it when we convert the Ion ICs.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00c4f6b6f300
part 1 - Add a generic proxy GETPROP stub to CacheIR. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/febcffbc2d4a
part 2 - Port code for GETPROP on shadowing DOM proxies to CacheIR. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d5568acc22e
part 3 - Support 64-bit fields in CacheIR stubs. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/90bd5f622e29
part 4 - Port code for unshadowed GETPROP on DOM proxies to CacheIR. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/873a10f77413
part 5 - Use CustomAutoRooter for CacheIRWriter. r=jonco

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #13)
> > +} JS_HAZ_GC_POINTER;
> 
> I'm unsure whether the analysis will think that use of this class over a
> possible GC is a hazard.  On the one hand it has this annotation (which is a
> hazard if it's not rooted) and on the other hand it is a CustomAutoRooter
> (which are ignored).  If you haven't done a hazard analysis try run yet I'd
> suggest you do one.

Ah yes, good catch. I removed the JS_HAZ_GC_POINTER annotation here (but kept the one on the StubField class), as you said it's not really necessary anymore with the CustomAutoRooter.

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/00c4f6b6f300
https://hg.mozilla.org/mozilla-central/rev/febcffbc2d4a
https://hg.mozilla.org/mozilla-central/rev/4d5568acc22e
https://hg.mozilla.org/mozilla-central/rev/90bd5f622e29
https://hg.mozilla.org/mozilla-central/rev/873a10f77413

Comment 23

[Boris Zbarsky [:bzbarsky]]

In the new setup, what replaces UpdateExistingGenerationalDOMProxyStub?

Comment 24

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #23)
> In the new setup, what replaces UpdateExistingGenerationalDOMProxyStub?

This is similar to the thing we used to do for getters, see bug 1310125 comment 3. Unfortunately we don't have a good mechanism for that yet. As that comment describes, before we attach a stub, we could check if there's an older stub with the same IR (that's the easy part, we can compare the CacheIRStubInfo* pointers for this), and then we can look at the IR instructions and check if it has the same ExpandoAndGeneration* guard but with a different generation value. For this we need to refactor the IR reader/format to make it easier to iterate/skip instructions. That's on my TODO list, I should be able to do this in a few weeks.

Comment 25

[Boris Zbarsky [:bzbarsky]]

OK.  This just has the potential to generate a _lot_ of stubs in real-life situations, because generation can update pretty often; it's a very coarse guard.  So it seems like we'd quickly run out of allowed stubs and deoptimize....

Is there a bug tracking this?

Comment 26

[Jan de Mooij [:jandem]]

Actually, I just realized I can pretty easily spotfix this just for the ExpandoAndGeneration thing for 53. I filed bug 1329195.