Allow arbitrary slot layouts on proxies

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: jandem)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 affected, firefox55 fixed)

Details

Attachments

(1 attachment)

We apparently don't have a bug on this....

This is needed for bug 1237503, at the very least, though I think mwu wanted this too.

Probably depends on bug 966518, right?
(Assignee)

Updated

2 years ago
Depends on: 1339411
Blocks: 1245974
Just today this made life significantly more complicated for the implementation of bug 556743: what should have been a single line of IDL is going to turn into a bunch of complexity and JSAPI usage...
(Assignee)

Comment 2

2 years ago
bz, what kind of API changes do you want to have here? I rewrote the proxy allocation code a while ago (bug 1339411) so this may be a bit easier now. Not sure if this actually depends on bug 966518.
Flags: needinfo?(bzbarsky)
> bz, what kind of API changes do you want to have here?

Well, in an ideal world I could have a proxy and non-proxy that have identical slot layouts in the sense that a given slot index can be used to get at the data in that slot without knowing whether the object involved is a proxy or not.

In a less-ideal world, I could live with something that checks for proxy vs not and converts the indices into slot addresses differently.  We are already living with that sort of thing in the form of js::GetReservedOrProxyPrivateSlot...  It would mean more branches and worse performance, of course.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 4

2 years ago
I don't think this depends on bug 966518 and it shouldn't be too hard. I'll try something.
Flags: needinfo?(jdemooij)
(Assignee)

Updated

2 years ago
No longer depends on: 966518
(Assignee)

Updated

2 years ago
Depends on: 1358753
(Assignee)

Comment 5

2 years ago
We have a plan for this but we need to check if it's compatible with JS_TransplantObject...
(Assignee)

Comment 6

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #5)
> We have a plan for this but we need to check if it's compatible with
> JS_TransplantObject...

See bug 1358753 comment 4. We have to keep ProxyValueArray, but after that bug lands hopefully we can rewrite ProxyValueArray to hold an array of n Values, where n is the number of reserved slots the Class wants.

That will require a bunch of changes to the proxy/wrapper classes, but most of the initial plan should still work.
(Assignee)

Comment 7

2 years ago
Hm I just realized there's also the following option:

* We keep ProxyValueArray* for each proxy, but we make it point to an array of n values (n is the number of reserved slots we get from the Class).
* At offset -1 we store the proxy's private Value.

This gives us some benefits:

* It's easier to implement because we keep the private slot we have now.
* Reserved slot x is stored at values[x], making it compatible with native objects.
* DOM proxies can store the expando in the private slot (offset -1), so we don't need to add the callback we discussed.

Does that make sense?
Flags: needinfo?(bzbarsky)
> * Reserved slot x is stored at values[x], making it compatible with native objects.

So this would be compatible with a native object that has some number of reserved slots but zero fixed slots, right?  That's not a thing at the moment; we would be creating it....

We would need to adjust the ion bits that do slot reads to do some sort of dynamic switching on the actual fixed slot count instead of just using the static "max fixed slots" value.

I guess that's all doable.  Doesn't help us get rid of the extra allocation...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 9

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #8)
> So this would be compatible with a native object that has some number of
> reserved slots but zero fixed slots, right?

Right, but at least you'd be able to use GetReservedSlot etc on both objects because that checks numFixedSlots which will be 0 for proxies, so it will do the right thing.

> We would need to adjust the ion bits that do slot reads to do some sort of
> dynamic switching on the actual fixed slot count instead of just using the
> static "max fixed slots" value.

Yeah, but often it will know statically whether the object is definitely (not) a proxy.

> I guess that's all doable.  Doesn't help us get rid of the extra
> allocation...

Bug 1358753 eliminates the allocation (except for JSObject::swap), but yes there's still a pointer dereference. Not sure if there's a better way to deal with JSObject::swap, I don't have time to go down that rabbit hole now. I'm happy enough we can get rid of malloc in the general case :)
> Right, but at least you'd be able to use GetReservedSlot etc

Indeed.

> Yeah, but often it will know statically whether the object is definitely (not) a proxy.

True.  This works for me, assuming we adjust the jitcode and all that.

> Bug 1358753 eliminates the allocation (except for JSObject::swap)

Ah, great!  So the slots are actually kinda fixed in practice in a lot of cases, but we sort of lie about whether they are.  That seems fine; we should just document things carefully.

I can live with JSObject::swap having to allocate, obviously.  ;)
Oh, we will presumably need to figure out how to address bug 1358596 if we do not in fact have fixed slots on proxies as far as the JIT is concerned.  We could presumably emit somewhat different MIR or something when we detect we might have a proxy?
(Assignee)

Comment 12

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #11)
> We could presumably emit somewhat different MIR or something when we detect
> we might have a proxy?

As a start we could use MGetDOMMember and MGetDOMProperty only when all objects are either definitely proxies or definitely not proxies (IonBuilder should know that). We could then add a |bool isProxy| to these MIR instructions and always load from ProxyValueArray if that's true.
Given that right now we don't even have any proxy classes capable of using MGetDOMMember/MGetDOMProperty, that seems reasonable.
(Assignee)

Comment 14

2 years ago
Posted patch PatchSplinter Review
This patch makes the proxy changes we discussed (more or less):

* The number of slots in ProxyValueArray is now dynamic and depends on the number of reserved slots we get from the Class.
* "Extra slots" was renamed to "Reserved slots" to make this clearer.
* All proxy Classes now have 2 reserved slots, but it should be easy to change that for proxy Classes that need more than 2 slots.
* Proxies now store a pointer to these slots and this means GetReservedSlot and SetReservedSlot can be used on proxies as well. We no longer need GetReservedOrProxyPrivateSlot and SetReservedOrProxyPrivate.

And some changes to make DOM Proxies work with this:

* We now store the C++ object in the first reserved slot (DOM_OBJECT_SLOT) instead of in the proxy's private slot. This is nice because it matches what we do for non-proxy DOM objects.
* We now store the expando in the proxy's private slot so I removed GetDOMProxyExpandoSlot and changed the IC code to get the expando from the private slot instead.

It's pretty large but most of the changes are very mechanical. Looks green on Try so far.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8861910 - Flags: review?(jcoppeard)
Attachment #8861910 - Flags: review?(bzbarsky)
(Assignee)

Comment 15

2 years ago
Comment on attachment 8861910 [details] [diff] [review]
Patch

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

::: js/public/Proxy.h
@@ +379,5 @@
> +// Proxy objects store a pointer to the reserved slots (ProxyReservedSlots*).
> +// The ProxyValueArray and the private slot can be accessed using
> +// ProxyValueArray::fromReservedSlots or ProxyDataLayout::values.
> +//
> +// Storing a pointer to ProxyReservedSlots* instead of ProxyValueArray* has a

I just noticed I should s/*// on this line..
Comment on attachment 8861910 [details] [diff] [review]
Patch

This looks great overall!  Thank you for doing this.

It's a bit annoying that in CreateBindingJSObject in Codegen.py we have to root expandoValue even though we know it's never a gcthing...  I guess we could cheat and HandleValue::fromMarkedLocation here if we think the performance difference is observable.  I'm not sure it is.

>+++ b/js/public/Proxy.h

We're using offsetof(ProxyValueArray, reservedSlots) in a few places; any reason to not just use offsetOfReservedSlots() consistently?

>+++ b/js/src/jsfriendapi.cpp
>+js::SetReservedSlotWithBarrier(JSObject* obj, size_t slot, const js::Value& value)

This is still ending up with an IsProxy() branch.  That's fine for now, but it seems like we should be able to get rid of that, since the slot layouts are now the same, right?  Followup is best for this.  Of course SetProxyReservedSlot will still be faster, because it won't need to check nfixed.

Speaking of which, comparing what NativeObject::setSlot and ProxyObject::setReservedSlot do, should our slots in the proxy be HeapSlot instead of Value?  Seems like it would get us barriering without the manual bits we have now.  Again, looks like followup fodder.

>+++ b/js/src/jsfriendapi.h

Might be worth saying in the comments for Get/SetReservedSlot that they're NOT OK to use for non-proxy non-native objects.

>+            JSCLASS_HAS_RESERVED_SLOTS(2) |                                             \

This works for now, but if a caller passes JSCLASS_HAS_RESERVED_SLOTS(n) for the flags, this will effectively make the number of slots (n | 2), right?  That's not fatal, but it might be worth a comment explaining that this is OK, because at worst it adds two slots the consumer will never touch to the requested slot count.  And for the common DOM case, where n == 1, it might be worth fixing at some point.  Followup is fine for that part.

>+++ b/js/src/jsobj.cpp
>+ProxyObject::initExternalValueArrayAfterSwap(JSContext* cx, const Vector<Value>& values)
>+    data.reservedSlots = &valArray->reservedSlots;

Can we assert that data.reservedSlots was null before this or something (to catch leaks)?  Or is it simply not initialized before this point?

>+++ b/js/src/proxy/Proxy.cpp
>@@ -680,24 +680,27 @@ ProxyObject::trace(JSTracer* trc, JSObje
>+        if (proxy->is<CrossCompartmentWrapperObject>() && i == 1)

I guess we can't replace the 1 with ProxyObject::grayLinkReservedSlot because this might be a IsDeadProxyObject() so we'll fail the assert?  Can we still common up the two by having a non-asserting version or something?  I assume that this 1 is exactly meant to match that 1, right?

>+++ b/js/src/vm/ProxyObject.cpp
>+    MOZ_ASSERT(nslots <= NativeObject::MAX_FIXED_SLOTS);

There's absolutely nothing that actually enforces that at runtime or compile-time, right?  So we're basically assuming we will have test coverage for any codepath that might allocate such objects.

That doesn't seem totally insane, but given that we already support proxies with non-inline ProxyValueArray, we could just use that support here in the cases when we would not fit in MAX_FIXED_SLOTS.  Again, followup is fine.

>+++ b/js/src/vm/Shape.h
>+        uint32_t free = clasp->isProxy() ? 0 : JSSLOT_FREE(clasp);

Can we make JSSLOT_FREE an inline function that asserts !clasp->isProxy()?  Seems like it would be a good idea...

r=me
Attachment #8861910 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #16)
> It's a bit annoying that in CreateBindingJSObject in Codegen.py we have to
> root expandoValue even though we know it's never a gcthing...  I guess we
> could cheat and HandleValue::fromMarkedLocation here if we think the
> performance difference is observable.  I'm not sure it is.

Note that the patch does remove a Rooted from CreateProxyObject so it's basically just moving it into the callers.

> Speaking of which, comparing what NativeObject::setSlot and
> ProxyObject::setReservedSlot do, should our slots in the proxy be HeapSlot
> instead of Value?  Seems like it would get us barriering without the manual
> bits we have now.  Again, looks like followup fodder.

Yeah, I'm not sure about it. There's a bunch of StoreBuffer stuff for HeapSlots/NativeObjects because these slots can move in memory, but that doesn't apply to proxies.

> 
> >+            JSCLASS_HAS_RESERVED_SLOTS(2) |                                             \
> 
> This works for now, but if a caller passes JSCLASS_HAS_RESERVED_SLOTS(n) for
> the flags, this will effectively make the number of slots (n | 2), right? 
> That's not fatal, but it might be worth a comment explaining that this is
> OK, because at worst it adds two slots the consumer will never touch to the
> requested slot count.  And for the common DOM case, where n == 1, it might
> be worth fixing at some point.  Followup is fine for that part.

Yeah the |2| is just temporary. I'll file a followup to make this dynamic (probably an additional macro argument) and change it to 1 for DOM proxies. I think that means we will go from a 4-slots GC thing to 2-slots, so it's pretty sweet.

> >+++ b/js/src/jsobj.cpp
> >+ProxyObject::initExternalValueArrayAfterSwap(JSContext* cx, const Vector<Value>& values)
> >+    data.reservedSlots = &valArray->reservedSlots;
> 
> Can we assert that data.reservedSlots was null before this or something (to
> catch leaks)?  Or is it simply not initialized before this point?

Good point. We will *only* get here when we swap a proxy that stored inline values. This code is called after we swap the main object fields, so at this point reservedSlots points into the old object. It should not leak AFAICS. I'll add a comment.

> I guess we can't replace the 1 with ProxyObject::grayLinkReservedSlot
> because this might be a IsDeadProxyObject() so we'll fail the assert?  Can
> we still common up the two by having a non-asserting version or something? 
> I assume that this 1 is exactly meant to match that 1, right?

Yeah, good idea, I'll file a follow-up for that too.

> >+++ b/js/src/vm/ProxyObject.cpp
> >+    MOZ_ASSERT(nslots <= NativeObject::MAX_FIXED_SLOTS);
> 
> There's absolutely nothing that actually enforces that at runtime or
> compile-time, right?  So we're basically assuming we will have test coverage
> for any codepath that might allocate such objects.
> 
> That doesn't seem totally insane, but given that we already support proxies
> with non-inline ProxyValueArray, we could just use that support here in the
> cases when we would not fit in MAX_FIXED_SLOTS.  Again, followup is fine.

I think asserting here is okay, but if we're really paranoid we could MOZ_RELEASE_ASSERT or MOZ_DIAGNOSTIC_ASSERT? I'm not too happy about using the non-inline code for this because it complicates nursery allocation a bit (swapped objects are not in the nursery so it's not a problem there). We could say if you want that many slots you don't get nursery allocation of course... But still it's a bunch of extra code we don't need (yet).

Thanks for the review, great comments!
(Assignee)

Comment 18

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #17)
> I think asserting here is okay, but if we're really paranoid we could
> MOZ_RELEASE_ASSERT or MOZ_DIAGNOSTIC_ASSERT?

I'll check if we can also static_assert this somehow when defining the proxy's class, that would be ideal.
> There's a bunch of StoreBuffer stuff for HeapSlots/NativeObjects because these
> slots can move in memory, but that doesn't apply to proxies

Why not, for the inline slots case?  Do we not move proxies in memory?  I'd think we do, at least in the nursery to non-nursery case.

> I'm not too happy about using the non-inline code for this because it complicates nursery allocation a bit

Ah, ok.  If there's a downside to using the non-inline code, then this is fine.  Might be worth documenting where PROXY_CLASS macros are defined.
Comment on attachment 8861910 [details] [diff] [review]
Patch

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

Looks good.  r=me for the JS parts.

::: js/public/Proxy.h
@@ +414,5 @@
> +    static size_t sizeOf(size_t nreserved) {
> +        return offsetof(ProxyValueArray, reservedSlots) + nreserved * sizeof(Value);
> +    }
> +    static MOZ_ALWAYS_INLINE ProxyValueArray* fromReservedSlots(ProxyReservedSlots* slots) {
> +        return (ProxyValueArray*)(uintptr_t(slots) - offsetof(ProxyValueArray, reservedSlots));

nit: should probably use reinterpret_cast here instead of C-style cast.

::: js/src/jsobj.cpp
@@ +1541,5 @@
> +
> +    js::detail::ProxyValueArray* valArray = js::detail::GetProxyDataLayout(proxy)->values();
> +    sb.unputValue(&valArray->privateSlot);
> +    if (!values.append(valArray->privateSlot))
> +        return false;

How about reserving space in the vector in advance and using infallible Append?
Attachment #8861910 - Flags: review?(jcoppeard) → review+
(In reply to Jan de Mooij [:jandem] from comment #17)
> Yeah, I'm not sure about it. There's a bunch of StoreBuffer stuff for
> HeapSlots/NativeObjects because these slots can move in memory, but that
> doesn't apply to proxies.

I think GCPtrValue makes the most sense for these.
(Assignee)

Updated

2 years ago
Blocks: 1360520
(Assignee)

Updated

2 years ago
Blocks: 1360523

Comment 22

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a3fcaa99ef
Refactor proxy slot layout to allow proxies to have more than 2 slots. r=bz,jonco
https://hg.mozilla.org/mozilla-central/rev/42a3fcaa99ef
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 years ago
Blocks: 1373527
(Assignee)

Updated

2 years ago
Blocks: 1113014
Duplicate of this bug: 1113014
You need to log in before you can comment on or make changes to this bug.