In local profiles, I'm seeing about 23ms spent in ComponentLoaderInfo::EnsureScriptChannel before firstpaint. That's happening because we create a channel for the URI passed to Cu.import every time it's called, regardless of whether the module is cached. We also do a lot of unnecessary extra work to resolve the URI to a local file in those cases. With a few quick hacks, I see that overhead all but disappear, so I think this is something we should be able to easily fix.

Comment on attachment 8888966 [details] Bug 1383215: Part 1 - Don't resolve module URIs to files when already cached. https://reviewboard.mozilla.org/r/159980/#review165358 It is odd that it was done this way...

Comment on attachment 8888967 [details] Bug 1383215: Part 2 - Split out URI resolution code into ResolveURI helper. https://reviewboard.mozilla.org/r/159982/#review165370 ::: startupcache/StartupCacheUtils.cpp:221 (Diff revision 1) > - > - nsCOMPtr<nsIResProtocolHandler> irph(do_QueryInterface(ph, &rv)); > - NS_ENSURE_SUCCESS(rv, rv); > > - rv = irph->ResolveURI(in, spec); > + nsCOMPtr<nsIURI> uri; > + rv = ResolveURI(in, getter_AddRefs(uri)); The new code was calls GetSpec() in the resource case, but the old code did not. Is there some reason that's okay?

Comment on attachment 8888968 [details] Bug 1383215: Part 3 - Use scache::ResolveURI to resolve module URIs. https://reviewboard.mozilla.org/r/159984/#review165378

Comment on attachment 8888967 [details] Bug 1383215: Part 2 - Split out URI resolution code into ResolveURI helper. https://reviewboard.mozilla.org/r/159982/#review165370 > The new code was calls GetSpec() in the resource case, but the old code did not. Is there some reason that's okay? The old code just used the spec returned from nsIResProtocolHandler::ResolveURI prior to it being converted into a URI. We wind up with the same spec and URI now as we did before, albeit slightly less efficienty for the resource: URI case.

Comment on attachment 8888967 [details] Bug 1383215: Part 2 - Split out URI resolution code into ResolveURI helper. https://reviewboard.mozilla.org/r/159982/#review165926

For part 3, if mKey is equal to mLocation, can you delete mKey and EnsureKey, and make Key() return mLocation (and change the return type)?

(In reply to Andrew McCreight [:mccr8] from comment #10) > For part 3, if mKey is equal to mLocation, can you delete mKey and > EnsureKey, and make Key() return mLocation (and change the return type)? I was considering getting rid of mKey, yeah. I decided against it, but not for any particular reason, so I'm happy to make that change. I'd like to keep EnsureKey and just make it an alias for EnsureLocation, though, just in case we decide to change it later.

(In reply to Kris Maglione [:kmag] from comment #11) > I was considering getting rid of mKey, yeah. I decided against it, but not > for any particular reason, so I'm happy to make that change. > > I'd like to keep EnsureKey and just make it an alias for EnsureLocation, > though, just in case we decide to change it later. Sounds good, thanks.

Comment on attachment 8888969 [details] Bug 1383215: Part 4 - Use location string as key in modules map. https://reviewboard.mozilla.org/r/159986/#review165930 ::: js/xpconnect/loader/mozJSComponentLoader.h:148 (Diff revision 1) > > nsCOMPtr<xpcIJSGetFactory> getfactoryobj; > JS::PersistentRootedObject obj; > JS::PersistentRootedScript thisObjectKey; > char* location; > + nsCString resolvedURL; Please get rid of the extra spacing that sort of but not really lines up the fields, for resolveURL and location.

https://hg.mozilla.org/integration/mozilla-inbound/rev/188f217f41e6eccc06ebf3cad6fd00d8a2bc390c Bug 1383215: Part 1 - Don't resolve module URIs to files when already cached. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/93ef01e8aee53dc19814a9040fd503853fe12bae Bug 1383215: Part 2 - Split out URI resolution code into ResolveURI helper. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/5cfde6ac518b8a8704c090f4f46bbb0996665b72 Bug 1383215: Part 3 - Use scache::ResolveURI to resolve module URIs. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/03777f604c6ca9dd56de4d8314284f7303fda46b Bug 1383215: Part 4 - Use location string as key in modules map. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/df302a197bca4990e47bce4b31dd00d051e7f079 Bug 1383215: Part 5 - Update tests that relied on loading the same JSM from multiple URLs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/50f76943b7cef407c6a2b2da994f91354a1d1d23 Bug 1383215: Follow-up: Fix straggler xpcshell test.

https://hg.mozilla.org/mozilla-central/rev/188f217f41e6 https://hg.mozilla.org/mozilla-central/rev/93ef01e8aee5 https://hg.mozilla.org/mozilla-central/rev/5cfde6ac518b https://hg.mozilla.org/mozilla-central/rev/03777f604c6c https://hg.mozilla.org/mozilla-central/rev/df302a197bca https://hg.mozilla.org/mozilla-central/rev/50f76943b7ce

this shows a perf improvement on android: == Change summary for alert #8229 (as of July 25 2017 03:59 UTC) == Improvements: 2% remote-blank summary android-4-2-armv7-api15 opt 1,516.28 -> 1,478.55 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8229'

and a bit of a talos win on desktop: == Change summary for alert #8239 (as of July 25 2017 03:59 UTC) == Improvements: 2% ts_paint_webext windows7-32 opt e10s 991.33 -> 967.42 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8239

Doesn't this break legacy add-ons? This caused TB a lot of bustage since we were using imports inconsistently (bug 1384218). One of my add-ons also got broken since it used Components.utils.import('resource://gre/modules/iteratorUtils.jsm'); Components.utils.import('resource://gre/modules/mailServices.js'); where the rest of TB uses this without the "gre". Wouldn't that also break FF add-ons when this ships in FF 56 beta and then in release?

Should this wait for 57? Given that it also broke a ton of tests, that seems a little concerning.

(In reply to Andrew McCreight [:mccr8] from comment #20) > Should this wait for 57? Given that it also broke a ton of tests, that seems > a little concerning. I don't think so. Nearly all of the tests it broke were xpcshell tests that were importing resource:///modules/Services.jsm or resource:///modules/XPCOMUtils.jsm, which we stopped supporting ordinary Firefox builds years ago. The others were just tests for the old module aliasing behavior, and one SDK loader test which only broke because it was testing the behavior of the loader separately for file: URLs and resource: URLs that point to the same place, but without unloading the module in-between.

(In reply to Kris Maglione [:kmag] from comment #21) > (In reply to Andrew McCreight [:mccr8] from comment #20) > > Should this wait for 57? Given that it also broke a ton of tests, that seems > > a little concerning. > > I don't think so. Nearly all of the tests it broke were xpcshell tests that > were importing resource:///modules/Services.jsm or > resource:///modules/XPCOMUtils.jsm, which we stopped supporting ordinary > Firefox builds years ago. Thunderbird does still allow this, though, so it may affect Thunderbird add-ons, but those aren't really affected by the 57 timeline one way or the other.

Sorry about the (silly) question: What does the "gre" stand for? No explanation here: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.import Any idea why TB is using imports of its own modules without the "gre"?

(In reply to Jorg K (GMT+2) from comment #23) > Sorry about the (silly) question: What does the "gre" stand for? No > explanation here: > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/ > Language_Bindings/Components.utils.import > > Any idea why TB is using imports of its own modules without the "gre"? There are two ways that we currently support packaging omnijar: 1) Separate JAR files for toolkit (GRE) content and app-specific content. 2) One JAR file containing both app-specific and toolkit content. Firefox uses the former (but used to use the latter), and Thunderbird uses the latter. In case 2, resource:/// and resource://gre/ point to the same place, so it's technically possible to refer to app or toolkit content by two separate URLs, and it's easy to carelessly use the wrong one. We had a bunch of these issues (especially with add-ons) when we switched layouts. But the code that's using resource://gre/ URLs for app content, or vice versa, is still technically wrong.

Thanks for the explanation. In bug 1384218 I fixed a lot of the "technically wrong": I fixed a few resource:///modules/Services.jsm and resource:///modules/XPCOMUtils.jsm by adding the "gre" and removed the "gre" from a URLs for TB/app content.

This also had a 20-30% improvement on ts_paint and a 10-20ms improvement on sessionrestore, but it looks like those tests are noisy enough at this point that it's not going to trigger an alert: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=853cfa601a8f4d94f94af50505e2960624e1ca12&newProject=mozilla-inbound&newRevision=df302a197bca4990e47bce4b31dd00d051e7f079&framework=1&showOnlyImportant=0

The changes from here made it so that it's no longer possible to open HTML pages from jetpack addons as regular web pages. (I'm getting "Access to the file was denied" since Nightly 20170726100241) Is this intentional? I'm interested to know because my addon uses a packaged HTML file as its UI, so it depends on the old behavior to work properly. I backed out all the changesets from here and my addon works again. Here is the error in the console when this happens: NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIWebNavigation.loadURIWithOptions] browser-child.js:354