From bug 1425466: (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #5) > (In reply to :Gijs from comment #4) > > (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're > > blocked) from comment #3) > > > (In reply to :Gijs from comment #0) > > > > 3078 Calling native method equals on iface nsIJSID with 2 args > > > > > > These are probably from XPCOMUtils.generateQI. We could probably add a > > > native helper for that. From what I remember of the startup profiles I > > > looked at, I don't think it would save more than 10ms. > > > > AIUI, if we fix 1384727, these ids will just be ints that we could compare > > directly in JS without ever going into native code. > > Unlikely. We'd probably still have nsIID objects, but they'd have numeric > IDs rather than UUID IDs, and even accessing those would require an > XPConnect call. > > > In any case, even if it's "only 10ms", I'm not 100% sure how reliable > > sample-based profiling is for stuff that is this fast individually, but used > > this frequently. I could be wrong, though. It certainly seems like something > > that we could improve, and 10ms on startup seems meaningful enough if it > > doesn't require rearchitecting the world. > > Just thinking in terms of cost-benefit analysis. It probably wouldn't be a > huge amount of work to replace XPCOMUtils.generateQI with a native version, > though. And we could probably even fast path that case when QIing an > XPCWrappedJS. I'm not sure off-hand how this would work. Would we come up with a convention for a name for a JS property that contains an array of iids, and would the xpconnect glue fetch the list of things and then 'catch' any QI calls? Would this matter for XPCOM that's only-in-JS? (ISTR from talking to Bobby that JS-to-JS-component XPCOM will in some cases still go through C++ because $reasons) Are there alternative options? I kind of want to repeat/rephrase my earlier question in bug 1425466 comment 0: given a current JS QI implementation which returns `this` for any "accepted" iid, and throws NO_INTERFACE for everything else... ... what would go wrong if we replaced the QI implementation with just: QueryInterface(/*unused*/) { return this; } ? Asking because that seems... easier. :-) I was under the impression xpconnect would save our ... if someone ever ended up QI'ing a JS object to an interface it didn't fully implement (ie it'd return non-NS_OK instead of crashing wildly when calling interface methods that weren't supported), and I can't think of a reasonable scenario where C++ would somehow get hold of a JS object and QI it to random things it doesn't support. But perhaps my imagination is not strong enough or I'm missing something obvious - it's Friday afternoon...

The other thing I just noticed in bug 1425466 is how we have hundreds of calls to nsIJSID.name, which is because the generateQI implementation in XPCOMUtils basically converts names to strings, and then later references the actual IDs based on Ci[name] notation inside the actual QI implementation it returns to consumers. I wonder how much we'd gain by just enforcing people pass strings instead of nsIJSID instances, which would avoid those .name lookups at the time of the generateQI call, as well as avoiding the Ci.<whatever> lookups from the callsite (which also involve xpconnect calls...). This is probably super-rewritable (as in, we can use a script to rewrite the codebase uses of this stuff), so it'd be pretty easy, plus it'll likely make things more readable... Alternatively, we could get rid of the string variation, or try to change how we check whether a thing is an interface or a string. Of course, all of that is kind of moot if we want to replace this stuff with something in C++ anyway. If we think that's hard, let me know and I can file a separate bug to make generateQI slightly smarter.

> and I can't think of a reasonable scenario where C++ would somehow get hold of a JS object > and QI it to random things it doesn't support It could QI it to things to check whether it supports them... This is not uncommon for C++ to do, actually. In terms of what we could do here... it depends on what use cases we want to support. The current XPConnect QI setup is very liberal and allows things like tearoffs, conditional decisions as to what interfaces are supported, etc. Of course none of this flexibility is needed for the generateQI case. One thing we could do is have generateQI both define a queryInterface function as it does now (to handle direct JS-to-JS things that for some reason don't go through XPCWrappedNative) and also define a property that stores the relevant IIDs in a property. The queryInterface impl could basically .includes() on the value of that property. We could teach XPCWrappedJS to get that property (possibly caching the result, if we want to) and work with that. If we do this, we'd want to either drop the string version or only support the string version or something...

(In reply to :Gijs from comment #0) > From bug 1425466: > > (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're > blocked) from comment #5) > > (In reply to :Gijs from comment #4) > > > (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're > > > blocked) from comment #3) > > > > (In reply to :Gijs from comment #0) > > > > > 3078 Calling native method equals on iface nsIJSID with 2 args > > > > > > > > These are probably from XPCOMUtils.generateQI. We could probably add a > > > > native helper for that. From what I remember of the startup profiles I > > > > looked at, I don't think it would save more than 10ms. > > > > > > AIUI, if we fix 1384727, these ids will just be ints that we could compare > > > directly in JS without ever going into native code. > > > > Unlikely. We'd probably still have nsIID objects, but they'd have numeric > > IDs rather than UUID IDs, and even accessing those would require an > > XPConnect call. > > > > > In any case, even if it's "only 10ms", I'm not 100% sure how reliable > > > sample-based profiling is for stuff that is this fast individually, but used > > > this frequently. I could be wrong, though. It certainly seems like something > > > that we could improve, and 10ms on startup seems meaningful enough if it > > > doesn't require rearchitecting the world. > > > > Just thinking in terms of cost-benefit analysis. It probably wouldn't be a > > huge amount of work to replace XPCOMUtils.generateQI with a native version, > > though. And we could probably even fast path that case when QIing an > > XPCWrappedJS. > > I'm not sure off-hand how this would work. Would we come up with a > convention for a name for a JS property that contains an array of iids, and > would the xpconnect glue fetch the list of things and then 'catch' any QI > calls? I though about that initially, but I think the simpler option would be to just replace XPCOMUtils.generateQI with a function that generated a native QueryInterface helper, and then have the XPConnect side check if the QueryInterface function of an XPCWrappedJS object was such a helper, and take a fast path, rather than going through XPConnect/JSAPI in the cases where it is. That should be the case the vast majority of the time for in-tree code these days. > I was under the impression xpconnect would save our ... if someone ever > ended up QI'ing a JS object to an interface it didn't fully implement (ie > it'd return non-NS_OK instead of crashing wildly when calling interface > methods that weren't supported) That's true... ish. If the native side tries to call a method that doesn't exist, we return NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED rather than crashing. But native code still uses QIs to determine if an object *should* implement certain interfaces (the most common/obvious being nsISupportsWeakReference), and may not handle things well if it claims to and then doesn't.

(In reply to :Gijs from comment #1) > I wonder how much we'd gain by just enforcing people pass strings instead of > nsIJSID instances, which would avoid those .name lookups at the time of the > generateQI call, as well as avoiding the Ci.<whatever> lookups from the > callsite (which also involve xpconnect calls...). This is probably > super-rewritable (as in, we can use a script to rewrite the codebase uses of > this stuff), so it'd be pretty easy, plus it'll likely make things more > readable... > > Alternatively, we could get rid of the string variation, or try to change > how we check whether a thing is an interface or a string. I would also be in favor of having only one way to call this instead of the current 2 supported ways. And yes, this sounds rewritable with a script, whichever variation of the two we decide to keep.

(In reply to Florian Quèze [:florian] from comment #4) > I would also be in favor of having only one way to call this instead of the > current 2 supported ways. And yes, this sounds rewritable with a script, > whichever variation of the two we decide to keep. I'm not especially concerned. That part isn't especially expensive. But if we go with one of the two, I'd prefer only supporting the string variant.