Closed
Bug 1131797
Opened 10 years ago
Closed 10 years ago
stop using parents for XPConnect's FixUpThisIfBroken
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: sfink, Assigned: bzbarsky)
References
Details
Attachments
(3 files)
7.88 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
3.13 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
3.44 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•10 years ago
|
||
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)
Reporter | ||
Comment 2•10 years ago
|
||
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.)
Comment 3•10 years ago
|
||
(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)
![]() |
Assignee | |
Comment 4•10 years ago
|
||
> 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.
Comment 5•10 years ago
|
||
(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.
![]() |
Assignee | |
Comment 6•10 years ago
|
||
> 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.
Comment 7•10 years ago
|
||
Ah I see. Thanks for explaining. :-)
Anyway, I'm fine either way - do whatever you prefer.
![]() |
Assignee | |
Comment 8•10 years ago
|
||
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.
Comment 9•10 years ago
|
||
(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 | |
Comment 10•10 years ago
|
||
Attachment #8570298 -
Flags: review?(bobbyholley)
![]() |
Assignee | |
Updated•10 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 11•10 years ago
|
||
Attachment #8570299 -
Flags: review?(bobbyholley)
![]() |
Assignee | |
Comment 12•10 years ago
|
||
Attachment #8570300 -
Flags: review?(bobbyholley)
Comment 13•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8570299 -
Flags: review?(bobbyholley) → review+
![]() |
Assignee | |
Comment 14•10 years ago
|
||
> 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?
![]() |
Assignee | |
Updated•10 years ago
|
Flags: needinfo?(bobbyholley)
Updated•10 years ago
|
Attachment #8570300 -
Flags: review?(bobbyholley) → review+
Comment 15•10 years ago
|
||
(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)
![]() |
Assignee | |
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1ccaf7d21b90
https://hg.mozilla.org/mozilla-central/rev/7aa34e8b4809
https://hg.mozilla.org/mozilla-central/rev/5f9adee38d45
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•