Stop using nsIXPCScriptable for ID objects

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [overhead:28k])

Attachments

(11 attachments)

46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
This should make them a touch more efficient to use & generally simplify some code. It's also a step on the way to eliminating consumers of nsIXPCScriptable.
Assignee

Comment 1

10 months ago
The new API tries to be more generic, taking and producing JS::Values. It also
supports creating the more specialized IID and CID types.
Assignee

Comment 2

10 months ago
This is the first part of hiding the implementation of nsIJSID behind the
interface added in Part 1, such that we can substitute that implementation out.

I had to make a couple of changes to fix the errors caused by the new behaviour
in GenerateQI.


Depends On D2278
Assignee

Comment 3

10 months ago
This should be mostly compatible with the original implementation. I tried to
simplify things here to just directly wrap and use JS objects, calling methods
on them. This eliminates the nsXPCConstructor type completely, replacing it with
a JSNative constructor with predefined READONLY | PERMANENT properties.


Depends On D2279
Assignee

Comment 4

10 months ago
Nobody was using it, so it's pretty easy to remove.


Depends On D2280
Assignee

Comment 5

10 months ago
This lets us get rid of the method 'initalize', which currently needs the
internal representation of JSCID.

This particular method is removed entirely in Part 9, and only exists to keep
intermediate states building & running.


Depends On D2281
Assignee

Comment 6

10 months ago
Rather than adding a native type for nsID objects in WebIDL, this patch just
takes the approach of switching consumers over to using 'any' and calling the
APIs defined in Part 1.


Depends On D2282
Assignee

Comment 7

10 months ago
This mostly consists of changes to the XPCComponents objects to avoid using the
implementation details, and instead use the API defined in part 1.


Depends On D2283
Assignee

Comment 8

10 months ago
These two interfaces are effectively never used, so to avoid needing to support
ClassID2JSValue with the new implementation, I remove them entirely.


Depends On D2284
Assignee

Comment 9

10 months ago
This is a complete rewrite of the interface while maintaining the same APIs.
Each ID is fully-contained within a single object, does not require a finalizer,
and is cheap to create.

Beyond using reserved slots, this code avoids using custom ClassOps, instead
preferring Symbol.hasInstance and eager constants.

One major change which occurred in this patch was the move from storing a nsCID
to storing the ContractID for JSCID objects. This eliminates the need for the
'refreshCID' method, and hopefully shouldn't have performance implications.

If we discover that there are performance problems there, we can look into
stashing the CID, and re-introduce 'refreshCID', despite its surprising
behaviour.


Depends On D2285
Assignee

Comment 10

10 months ago
(In reply to :Nika Layzell from comment #2)
> Created attachment 8993868 [details]
> Bug 1477432 - Part 2: Avoid using nsIJSID in GenerateQI, and produce better
> diagnostics, r=kmag
> 
> This is the first part of hiding the implementation of nsIJSID behind the
> interface added in Part 1, such that we can substitute that implementation
> out.
> 
> I had to make a couple of changes to fix the errors caused by the new
> behaviour
> in GenerateQI.
> 
> 
> Depends On D2278

Hey kmag, can you create a Phabricator account so I can flag you for review on this revision? :-) Thanks!
Flags: needinfo?(kmaglione+bmo)
(In reply to :Nika Layzell from comment #10)
> Hey kmag, can you create a Phabricator account so I can flag you for review
> on this revision? :-) Thanks!

Bah. Fine.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8993867 [details]
Bug 1477432 - Part 1: Move xpc_ nsJSID methods to a future-proof API,

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2278
Attachment #8993867 - Flags: review+
Comment on attachment 8993869 [details]
Bug 1477432 - Part 3: Avoid using nsIJSID in Components.Constructor,

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2280
Attachment #8993869 - Flags: review+
Comment on attachment 8993870 [details]
Bug 1477432 - Part 4: Remove the nsJSID XPCOM constructor,

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2281
Attachment #8993870 - Flags: review+
Comment on attachment 8993871 [details]
Bug 1477432 - Part 5: Add a refreshCID method to JSCID objects,

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2282
Attachment #8993871 - Flags: review+
There's a (recent) use of Components.interfacesByID in toolkit/content/customElements.js. I'm guessing that'll need to be dealt with in part 8.
Flags: needinfo?(nika)
Comment on attachment 8993873 [details]
Bug 1477432 - Part 7: Stop using nsIJSID in xpconnect outside of XPCJSID.cpp,

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2284
Attachment #8993873 - Flags: review+
Comment on attachment 8993872 [details]
Bug 1477432 - Part 6: Stop using nsIJSID inside of WebIDL bindings,

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2283
Attachment #8993872 - Flags: review+
Boris, is it okay to you to replace the usage of |IID| with |any| in WebIDL? (See part 6.) The IID conversion ends up being pushed down into the leaves a bit more.
Flags: needinfo?(bzbarsky)
It seems ok to me, but see my other comments on the revision.
Flags: needinfo?(bzbarsky)
Hm. It doesn't seem like it should be that hard to just have an ID type and have the binding layer convert to nsID& rather than pushing that onto the callees...
It's not that hard, but it takes a bit of codegen work and maybe IDL parser work depending on exactly how you set it up.  Given the use cases we have so far it's probably not worth the complexity.
Assignee

Comment 23

10 months ago
(In reply to Andrew McCreight [:mccr8] from comment #16)
> There's a (recent) use of Components.interfacesByID in
> toolkit/content/customElements.js. I'm guessing that'll need to be dealt
> with in part 8.

Interesting. It looks from what I can see like I should be able to make that work without issue :-). - I'll have to handle it in part 8 for sure though.

(In reply to Andrew McCreight [:mccr8] from comment #19)
> Boris, is it okay to you to replace the usage of |IID| with |any| in WebIDL?
> (See part 6.) The IID conversion ends up being pushed down into the leaves a
> bit more.

(In reply to Kris Maglione [:kmag] from comment #21)
> Hm. It doesn't seem like it should be that hard to just have an ID type and
> have the binding layer convert to nsID& rather than pushing that onto the
> callees...

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #22)
> It's not that hard, but it takes a bit of codegen work and maybe IDL parser
> work depending on exactly how you set it up.  Given the use cases we have so
> far it's probably not worth the complexity.

I understand. My original patch actually did that codegen work, but I figured it wasn't worth the complexity to do this in the codegen layer given the small number of consumers which would have to handle nsID objects. If we decide that we want to have support for nsID natively in webIDL, I can easily add it though.

--

I'll reply to the other comments on the revision on Phabricator :-)
Flags: needinfo?(nika)
Comment on attachment 8993875 [details]
Bug 1477432 - Part 9: Switch to using plain JS objects for nsIJS[IC]ID,

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2286
Attachment #8993875 - Flags: review+
Assignee

Updated

10 months ago
Priority: -- → P3
Attachment #8993867 - Attachment description: Bug 1477432 - Part 1: Move xpc_ nsJSID methods to a future-proof API, r=mccr8 → Bug 1477432 - Part 1: Move xpc_ nsJSID methods to a future-proof API,
Attachment #8993868 - Attachment description: Bug 1477432 - Part 2: Avoid using nsIJSID in GenerateQI, and produce better diagnostics, r=kmag → Bug 1477432 - Part 2: Avoid using nsIJSID in GenerateQI, and produce better diagnostics,
Attachment #8993869 - Attachment description: Bug 1477432 - Part 3: Avoid using nsIJSID in Components.Constructor, r=mccr8 → Bug 1477432 - Part 3: Avoid using nsIJSID in Components.Constructor,
Attachment #8993870 - Attachment description: Bug 1477432 - Part 4: Remove the nsJSID XPCOM constructor, r=mccr8 → Bug 1477432 - Part 4: Remove the nsJSID XPCOM constructor,
Attachment #8993871 - Attachment description: Bug 1477432 - Part 5: Add a refreshCID method to JSCID objects, r=mccr8 → Bug 1477432 - Part 5: Add a refreshCID method to JSCID objects,
Attachment #8993872 - Attachment description: Bug 1477432 - Part 6: Stop using nsIJSID inside of WebIDL bindings, r=mccr8 → Bug 1477432 - Part 6: Stop using nsIJSID inside of WebIDL bindings,
Attachment #8993873 - Attachment description: Bug 1477432 - Part 7: Stop using nsIJSID in xpconnect outside of XPCJSID.cpp, r=mccr8 → Bug 1477432 - Part 7: Stop using nsIJSID in xpconnect outside of XPCJSID.cpp,
Attachment #8993874 - Attachment description: Bug 1477432 - Part 8: Remove test-only Components.classesById and Components.interfacesById, r=mccr8 → Bug 1477432 - Part 8: Remove test-only Components.classesById and Components.interfacesById,
Attachment #8993875 - Attachment description: Bug 1477432 - Part 9: Switch to using plain JS objects for nsIJS[IC]ID, r=mccr8 → Bug 1477432 - Part 9: Switch to using plain JS objects for nsIJS[IC]ID,

Comment 27

6 months ago
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e250aa206d1
Part 1: Move xpc_ nsJSID methods to a future-proof API, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/404c4d583acd
Part 2: Avoid using nsIJSID in GenerateQI, and produce better diagnostics, r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecbe615201fa
Part 3: Avoid using nsIJSID in Components.Constructor, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b4c9cdfbd7
Part 4: Remove the nsJSID XPCOM constructor, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c019316fcfd4
Part 5: Add a refreshCID method to JSCID objects, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab47ae5672ac
Part 6: Stop using nsIJSID inside of WebIDL bindings, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a68bee9d2168
Part 7: Stop using nsIJSID in xpconnect outside of XPCJSID.cpp, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ddcdfc06526
Part 8: Remove test-only Components.classesById and Components.interfacesById, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3439b17bdc2a
Part 9: Switch to using plain JS objects for nsIJS[IC]ID, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/29c30b62c301
Part 10: Stop using nsIJSID in nsIArray for optional IID parameters, r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/241738774bb1
Part 11: Update CustomElementRegistry to not use nsIJSID, r=smaug
(In reply to Eric Rahm [:erahm] from comment #29)
> This seems to have regressed our base JS numbers by 28k [1].
> 
> [1]
> https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,
> 1684866,1,4&series=mozilla-inbound,1684802,1,4&series=autoland,1684884,1,
> 4&zoom=1542371243532.9512,1542492308031.5186,4500000,4563805.
> 9701492535&selected=mozilla-inbound,1684802,404817,645267204,4

It's possible that this increased our JS numbers, but decreased our overall memory usage by moving data from wrapped natives to native JS objects which are larger than the old reflectors.

It's also possible something else is going on, and we have more prototype or compartment overhead now.
(In reply to Kris Maglione [:kmag] from comment #30)
> It's possible that this increased our JS numbers, but decreased our overall
> memory usage by moving data from wrapped natives to native JS objects which
> are larger than the old reflectors.

erahm and I have discussed and it appears that this is what is happening here. This moves a bunch of data from the native heap to the JS heap.

This system should allocate ~the same number of JS objects as before (old objects should've had prototypes already).

Constants on nsIIDs are eagerly defined instead of resolved now. I think this won't have any significant impact because they're all integers, and there aren't that many of them. If we think this is a problem we could reintroduce a resolve hook.

In addition, we now hold a reference to the ContractID string used for indexing into Cc[...] rather than making a native const char* copy of it, which will move those strings into the JS heap.
Well, as far as prototypes go, they work differently for nohelper xpcscriptables than they do for native classes and WebIDL objects. Instead of each type having it's own prototype, each object has all of its own properties defined by resolve hooks. That usually means WebIDL and native classes use less memory, but sometimes the fact that each compartment needs it's own prototype means they wind up using more.

Anyway, I don't really know enough about the implementation of these ID objects to know if that could actually be going on here.

Updated

6 months ago
Depends on: 1511359
This caused regression bug 1511359 in Thunderbird. That bug appears to be a wider issue that probably would also affect WebExperiments for WebExtensions in Firefox. Could somebody please take a look at that?
You need to log in before you can comment on or make changes to this bug.