1383215 - Cu.import is far too slow for the cached module case

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Kris Maglione [:kmag]]

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 1

[Kris Maglione [:kmag]]

Attachment: Bug 1383215: Part 1 - Don't resolve module URIs to files when already cached.

Review commit: https://reviewboard.mozilla.org/r/159980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159980/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1383215: Part 2 - Split out URI resolution code into ResolveURI helper.

Review commit: https://reviewboard.mozilla.org/r/159982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159982/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1383215: Part 3 - Use scache::ResolveURI to resolve module URIs.

Since we now usually load modules from one of the startup caches, we usually
have no need to ever actually create a channel in order to load them.
Resolving the URIs directly is much cheaper in the normal case.

Review commit: https://reviewboard.mozilla.org/r/159984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159984/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1383215: Part 4 - Use location string as key in modules map.

Using the unmolested module location string as the cache key removes a huge
chunk of overhead when loading cached modules.

This also ensures that multiple URLs are not used to load the same module,
which would result in it being loaded more than once in the new regime

Review commit: https://reviewboard.mozilla.org/r/159986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159986/

Comment 5

[Andrew McCreight [:mccr8]]

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 6

[Andrew McCreight [:mccr8]]

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 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8888968 [details]
Bug 1383215: Part 3 - Use scache::ResolveURI to resolve module URIs.

https://reviewboard.mozilla.org/r/159984/#review165378

Comment 8

[Kris Maglione [:kmag]]

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 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8888967 [details]
Bug 1383215: Part 2 - Split out URI resolution code into ResolveURI helper.

https://reviewboard.mozilla.org/r/159982/#review165926

Comment 10

[Andrew McCreight [:mccr8]]

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)?

Comment 11

[Kris Maglione [:kmag]]

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

Comment 12

[Andrew McCreight [:mccr8]]

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

[Andrew McCreight [:mccr8]]

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.

Comment 14

[Kris Maglione [:kmag]]

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.

Comment 15

[Kris Maglione [:kmag]]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

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'

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 19

[Jorg K (CEST = GMT+2)]

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?

Comment 20

[Andrew McCreight [:mccr8]]

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

Comment 21

[Kris Maglione [:kmag]]

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

Comment 22

[Kris Maglione [:kmag]]

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

Comment 23

[Jorg K (CEST = GMT+2)]

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"?

Comment 24

[Kris Maglione [:kmag]]

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

Comment 25

[Jorg K (CEST = GMT+2)]

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.

Comment 26

[Kris Maglione [:kmag]]

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

Comment 27

[Alan]

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