Closed Bug 1131797 Opened 7 years ago Closed 7 years ago

stop using parents for XPConnect's FixUpThisIfBroken

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: sfink, Assigned: bzbarsky)

References

Details

Attachments

(3 files)

Split out from bug 805052, item 9: XPConnect's FixUpThisIfBroken.  We can probably replace this by functions with extra reservd slots which store the relevant object in those slots.
So turns out XPConnect functions already use the two extra reserved slots, to store the XPCNativeMember* and XPCNativeInterface*.

Some possible options:

1)  Add an XPCNativeInterface* to XPCNativeMember, pointing to its parent interface.  Then stop storing the XPCNativeInterface* in a slot and use that slot for the home object.

2)  Rip out FixUpThisIfBroken, though the comments say this is likely to break chrome (and I bet extension) code in various ways.

3)  Switch to using bound methods for the cases when FixUpThisIfBroken would be triggered.  This seems like it would use a bunch of extra memory on a per-method basis, right?

4)  Add another reserved slot to the return value of NewFunctionByIdWithReserved.  Again, seems like it uses more memory... but we might save that on being able to share more shapes across our function objects, maybe?

Mr. XPConnect owner?
Flags: needinfo?(bobbyholley)
5) Maintain a WeakMap from the funobj to its magic this value. Kind of scary for GC, since we don't handle WMs very well.

6) Add a property to the funobj, keyed by a private Symbol, holding the magic this value. (Does this pattern ever work yet? Perhaps we don't have private enough symbols yet. It'd be weird to be able to grab this symbol out and muck with other objects.)
(In reply to Not doing reviews right now from comment #1)
> So turns out XPConnect functions already use the two extra reserved slots,
> 1)  Add an XPCNativeInterface* to XPCNativeMember, pointing to its parent
> interface.  Then stop storing the XPCNativeInterface* in a slot and use that
> slot for the home object.

This seems like by far the simplest option. The downside is that it may swell memory somewhat. Can you implement a quick-and-dirty static counter an measure how many XPCNativeMembers are alive when the browser is running? IIRC these things are shared across globals, so it might not be so bad.

There are various clever things we could do to reduce the footprint, but let's measure first.


> 2)  Rip out FixUpThisIfBroken, though the comments say this is likely to
> break chrome (and I bet extension) code in various ways.

This is a non-starter, I guarantee it.

> 3)  Switch to using bound methods for the cases when FixUpThisIfBroken would
> be triggered.  This seems like it would use a bunch of extra memory on a
> per-method basis, right?

This seems like it would use a lot more memory than (4).

> 4)  Add another reserved slot to the return value of
> NewFunctionByIdWithReserved.  Again, seems like it uses more memory... but
> we might save that on being able to share more shapes across our function
> objects, maybe?

If we were able to do this only for the XPCWNNoHelper methods this would basically replicate the current setup with parents. But I don't know how this would change GC patterns, and (1) is still simpler.
Flags: needinfo?(bobbyholley)
> Can you implement a quick-and-dirty static counter an measure how many XPCNativeMembers
> are alive when the browser is running?

Sure thing.  I tried doing this, then opening and using various Firefox UI bits, logging only new high-water-mark values.  The last value I saw logged was 3530.

I didn't actually log XPCNativeMember per se, because we memcpy those around, but rather logged the number on the heap in XPCNativeInterfaces, which I think should be the same thing.

So anyway, if we figure 4k XPCNativeMembers, figure 16-32KB depending on whether we're 32-bit or 64-bit.

We could probably keep it down to 16KB by playing some games (e.g. storing the index into the mMembers array, which is actually guaranteed to fit in 16 bits, and doing some pointer arithmetic and offsetof() stuff to get the XPCNativeInterface*).  That would also mean we don't need to do a second pass over the mMembers after we allocate the XPCNativeInterface, but is a bit more convoluted.
(In reply to Not doing reviews right now from comment #4) 
> I didn't actually log XPCNativeMember per se, because we memcpy those
> around, but rather logged the number on the heap in XPCNativeInterfaces,
> which I think should be the same thing.
> 
> So anyway, if we figure 4k XPCNativeMembers, figure 16-32KB depending on
> whether we're 32-bit or 64-bit.

I think that is a reasonable price to pay for the benefit of getting rid of parents. 

> We could probably keep it down to 16KB by playing some games (e.g. storing
> the index into the mMembers array, which is actually guaranteed to fit in 16
> bits, and doing some pointer arithmetic and offsetof() stuff to get the
> XPCNativeInterface*).  That would also mean we don't need to do a second
> pass over the mMembers after we allocate the XPCNativeInterface, but is a
> bit more convoluted.

You mean 8k/16k, right? That actually sounds pretty reasonable with the appropriate helpers. I'd be for doing it, but happy to do the dumb thing if it's significantly easier.
> You mean 8k/16k, right?

No, I mean 16k.  Right now, XPCNativeMember has a jsid and two uint16_t.  That means that on 32bit it's 8 bytes and on 64bit it's 16 bytes (12 + padding to align things correctly).

If we add one more pointer, that becomes 12 bytes and 24 bytes respectively.

If we add a 16-bit offset field, that becomes 12 bytes (10 + padding) on 32bit and still 16 bytes (14 + padding) on 64bit (I miscalculated before and thought it was 20).  So the two approaches are identical on 32bit but using the offset thing is clearly better for memory usage on 64bit.
Ah I see. Thanks for explaining. :-)

Anyway, I'm fine either way - do whatever you prefer.
Actually, maybe I can even do better than that, since mFlags doesn't need a full 16 bits.  In fact it needs only 4.  Are you OK with limiting interfaces to no more than 2^12 members?  I think I am.
(In reply to Not doing reviews right now from comment #8)
> Actually, maybe I can even do better than that, since mFlags doesn't need a
> full 16 bits.  In fact it needs only 4.  Are you OK with limiting interfaces
> to no more than 2^12 members?  I think I am.

Yep.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8570298 [details] [diff] [review]
part 1.  Store an index into its XPCNativeInterface mMembers array in each XPCNativeMember

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

::: js/xpconnect/src/XPCInlines.h
@@ +256,5 @@
> +XPCNativeMember::GetInterface() const
> +{
> +    XPCNativeMember* arrayStart =
> +        const_cast<XPCNativeMember*>(this - mIndexInInterface);
> +    size_t arrayStartOffset = XPCNativeInterface::OffsetOfMembers();

Is there a reason we can't just inline the definition of OffsetOfMembers here?
Attachment #8570298 - Flags: review?(bobbyholley) → review+
Attachment #8570299 - Flags: review?(bobbyholley) → review+
> Is there a reason we can't just inline the definition of OffsetOfMembers here?

It would require either making XPCNativeMember a friend of XPCNativeInterface or making the mMembers member of XPCNativeInterface public.  Would you prefer either one of those to this setup?
Flags: needinfo?(bobbyholley)
Attachment #8570300 - Flags: review?(bobbyholley) → review+
(In reply to Not doing reviews right now from comment #14)
> > Is there a reason we can't just inline the definition of OffsetOfMembers here?
> 
> It would require either making XPCNativeMember a friend of
> XPCNativeInterface or making the mMembers member of XPCNativeInterface
> public.  Would you prefer either one of those to this setup?

Not particularly.
Flags: needinfo?(bobbyholley)
Blocks: 1131805
You need to log in before you can comment on or make changes to this bug.