The module loader interface is exposed to JS twice, once in Cu and once in xpcIJSModuleLoader, but nobody uses the latter. This leads to an annoying duplicated comment, and I'll have to fix that comment for bug 1186409, so let's just remove the cruft. I've also removed an overload of mozJSComponentLoader::ImportInto which is never called. There are only two methods left on xpcIJSModuleLoader, which were added recently for testing purposes, so this interface could be eliminated if those were also moved to Cu, but I think it is fine to have these testing-only methods here instead of bloating up Cu more.

Theoretically this could be an addon compat issue but a search on addons MXR only turned up a couple of references to the interface, and nothing substantial. Also, making this interface builtinclass could theoretically cause issues, but I can't imagine anybody implementing their own JS component loader in JS!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d19234409a58050188e42608c29f243980ff1b09

Comment on attachment 8887609 [details] Bug 1381919 - Remove most of the xpcIJSModuleLoader interface. https://reviewboard.mozilla.org/r/158486/#review163796 ::: js/xpconnect/src/XPCComponents.cpp:2498 (Diff revision 1) > - nsCOMPtr<xpcIJSModuleLoader> moduleloader = > + RefPtr<mozJSComponentLoader> moduleloader = mozJSComponentLoader::Get(); > - do_GetService(MOZJSCOMPONENTLOADER_CONTRACTID); Huh. So we've been going through an extra layer of COM gunk for every Cu.import call for all these years? :( ::: js/xpconnect/src/XPCComponents.cpp:2499 (Diff revision 1) > if (!moduleloader) > return NS_ERROR_FAILURE; Can we just make this an assertion now? These methods are only called from JS, so we should never be able to get here when there's no module loader.

(In reply to Kris Maglione [:kmag] from comment #4) > Huh. So we've been going through an extra layer of COM gunk for every > Cu.import call for all these years? :( Yep! Generally, we really overuse do_GetService. > Can we just make this an assertion now? These methods are only called from > JS, so we should never be able to get here when there's no module loader. That sounds fine.

(In reply to Andrew McCreight [:mccr8] from comment #5) > (In reply to Kris Maglione [:kmag] from comment #4) > > Huh. So we've been going through an extra layer of COM gunk for every > > Cu.import call for all these years? :( > Yep! Generally, we really overuse do_GetService. Is there a significant perf impact when using getService too often? Thinking mostly about our JS code, could we expect a significant win if we replaced (using a script) all the useless .getService calls with the Services.foo equivalents from Services.jsm? I've wanted to do that for a while, but considered it mostly a code cleanup. If we expect it to be a perf win I would make it happen sooner than later.

(In reply to Florian Quèze [:florian] [:flo] from comment #6) > Is there a significant perf impact when using getService too often? Thinking > mostly about our JS code, could we expect a significant win if we replaced > (using a script) all the useless .getService calls with the Services.foo > equivalents from Services.jsm? I've wanted to do that for a while, but > considered it mostly a code cleanup. If we expect it to be a perf win I > would make it happen sooner than later. There is, but the impact is different for JS than it is for C++. In JS, there are some extra layers of caching, so you're usually not actually calling the underlying getService logic or QueryInterface. But there's also all sorts of extra overhead any time you touch XPConnect. In C++, it's a different issue. A static getter like the one this patch moves to is basically free. do_GetService requires, among other things, a factory lookup for the contract ID, an expensive QueryInterface to get the correct interface from the module, and then virtual calls to the actual interface methods. That's orders of magnitude of extra overhead, as opposed to JS case where it's measurable, but a small part of the overall cost.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3b275d92b419 Remove most of the xpcIJSModuleLoader interface. r=kmag

Backed out for 32-bit Windows build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=115503167&repo=autoland 64-bit seems fine, non-windows seems fine, for the record. https://hg.mozilla.org/integration/autoland/rev/a4e7af47aa3a1de0d2c79dec6512963ee37c177a

I forgot to change the NS_IMETHODIMP to nsresult on the definitions of the methods I deCOMed.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/edca2b438807 Remove most of the xpcIJSModuleLoader interface. r=kmag