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

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {addon-compat})

unspecified
mozilla56
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(4 attachments)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
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...
Attachment #8888966 - Flags: review?(continuation) → review+

Comment 6

a year ago
mozreview-review
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?
Attachment #8888967 - Flags: review?(continuation) → review-

Comment 7

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

https://reviewboard.mozilla.org/r/159984/#review165378
Attachment #8888968 - Flags: review?(continuation) → review+
(Assignee)

Comment 8

a year ago
mozreview-review-reply
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.

Updated

a year ago
Whiteboard: [qf] → [qf:p1]

Comment 9

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

https://reviewboard.mozilla.org/r/159982/#review165926
Attachment #8888967 - Flags: review- → review+
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)?
(Assignee)

Comment 11

a year ago
(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 13

a year ago
mozreview-review
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.
Attachment #8888969 - Flags: review?(continuation) → review+
(Assignee)

Comment 14

a year ago
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.
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

Comment 19

a year ago
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.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 21

a year ago
(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.
Flags: needinfo?(kmaglione+bmo)
Keywords: addon-compat
(Assignee)

Comment 22

a year ago
(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

a year ago
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"?
(Assignee)

Comment 24

a year ago
(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

a year ago
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.
(Assignee)

Comment 26

a year ago
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
Depends on: 1384748

Comment 27

a year ago
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

Updated

a year ago
Depends on: 1388088
You need to log in before you can comment on or make changes to this bug.