1477432 - Stop using nsIXPCScriptable for ID objects

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 1: Move xpc_ nsJSID methods to a future-proof API,

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

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 2: Avoid using nsIJSID in GenerateQI, and produce better diagnostics,

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

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 3: Avoid using nsIJSID in Components.Constructor,

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

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 4: Remove the nsJSID XPCOM constructor,

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


Depends On D2280

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 5: Add a refreshCID method to JSCID objects,

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

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 6: Stop using nsIJSID inside of WebIDL bindings,

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

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 7: Stop using nsIJSID in xpconnect outside of XPCJSID.cpp,

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

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 8: Remove test-only Components.classesById and Components.interfacesById,

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

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 9: Switch to using plain JS objects for nsIJS[IC]ID,

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

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(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!

Comment 11

[Kris Maglione [:kmag]]

(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.

Comment 12

[Phabricator Automation]

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

Comment 13

[Phabricator Automation]

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

Comment 14

[Phabricator Automation]

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

Comment 15

[Phabricator Automation]

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

Comment 16

[Andrew McCreight [:mccr8]]

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.

Comment 17

[Phabricator Automation]

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

Comment 18

[Phabricator Automation]

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

Comment 19

[Andrew McCreight [:mccr8]]

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

It seems ok to me, but see my other comments on the revision.

Comment 21

[Kris Maglione [:kmag]]

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...

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Nika Layzell [:nika] (ni? for response)]

(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 :-)

Comment 24

[Phabricator Automation]

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

Comment 25

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 10: Stop using nsIJSID in nsIArray for optional IID parameters,

Depends on D2286

Comment 26

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1477432 - Part 11: Update CustomElementRegistry to not use nsIJSID,

Depends on D9732

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2e250aa206d1
https://hg.mozilla.org/mozilla-central/rev/404c4d583acd
https://hg.mozilla.org/mozilla-central/rev/ecbe615201fa
https://hg.mozilla.org/mozilla-central/rev/e3b4c9cdfbd7
https://hg.mozilla.org/mozilla-central/rev/c019316fcfd4
https://hg.mozilla.org/mozilla-central/rev/ab47ae5672ac
https://hg.mozilla.org/mozilla-central/rev/a68bee9d2168
https://hg.mozilla.org/mozilla-central/rev/1ddcdfc06526
https://hg.mozilla.org/mozilla-central/rev/3439b17bdc2a
https://hg.mozilla.org/mozilla-central/rev/29c30b62c301
https://hg.mozilla.org/mozilla-central/rev/241738774bb1

Comment 29

[Eric Rahm [:erahm]]

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

Comment 30

[Kris Maglione [:kmag]]

(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.

Comment 31

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 32

[Kris Maglione [:kmag]]

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.

Comment 33

[Ben Bucksch (:BenB)]

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?