1388215 - Use loops for lazy imports in files with a lot of them

Status: RESOLVED FIXED

(Toolkit :: General, enhancement)

Description

[Kris Maglione [:kmag]]

The JIT significantly speeds up the definition of lazy imports in cases where we define a lot of them at once.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1388215: Part 3 - Use JIT-friendly defineLazy*Getters methods when defining many lazy imports.

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

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8894704 [details]
Bug 1388215: Part 3 - Use JIT-friendly defineLazy*Getters methods when defining many lazy imports.

https://reviewboard.mozilla.org/r/165870/#review171080

::: toolkit/components/extensions/Extension.jsm:48
(Diff revision 1)
>  
>  /* globals processCount */
>  
>  XPCOMUtils.defineLazyPreferenceGetter(this, "processCount", "dom.ipc.processCount.extension");
>  
> -XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
> +/* globals AddonManager, AddonManagerPrivate, AsyncShutdown, ExtensionCommon, ExtensionPermissions, ExtensionStorage, ExtensionTestCommon, Log, MessageChannel, NetUtil, OS, Schemas, setTimeout, TelemetryStopwatch */

These comments are annoying to maintain, and these loops duplicated in each file are non trivial to parse with eslint. Would we get the same performance gain if we put the loop in XPCOMUtils and did instead:
XPCOMUtils.defineLazyModuleGetters(this, [
  ...
]);

?

Comment 3

[Shane Caraveo (:mixedpuppy)]

+1 on comment 2

Comment 4

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> These comments are annoying to maintain, and these loops duplicated in each
> file are non trivial to parse with eslint. Would we get the same performance
> gain if we put the loop in XPCOMUtils and did instead:
> XPCOMUtils.defineLazyModuleGetters(this, [
>   ...
> ]);
> 
> ?

Not quite the same performance gain. On the one hand, we'd only have to baseline compile the loop once, so we'd be able to use it in places with fewer imports. On the other hand, the JITs don't handle cross-compartment wrappers well, so the loops would probably be slower, but the defineLazyGetter calls would probably be faster.

But if we passed an object, rather than an array of arrays, and used Object.entries, that would probably make the JIT happy at the expense of generating a bit more garbage.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] from comment #4)
> But if we passed an object, rather than an array of arrays, and used
> Object.entries, that would probably make the JIT happy at the expense of
> generating a bit more garbage.

Yeah, with this approach it seems to execute within the minimum sampling interval with any number of scripts we currently pass it.

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1388215: Part 1 - Add defineLazyModuleGetters and defineLazyServiceGetters methods.

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

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1388215: Part 2 - Add eslint plugin support for defineLazy*Getters() methods.

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

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8894704 [details]
Bug 1388215: Part 3 - Use JIT-friendly defineLazy*Getters methods when defining many lazy imports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165870/diff/1-2/

Comment 9

[Kris Maglione [:kmag]]

So far these don't show the clear performance improvements on try that I was seeing locally, but it does make a lot of code cleaner, and a 10ms ts_paint improvement is within the noise, but should still add up.

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8894704 [details]
Bug 1388215: Part 3 - Use JIT-friendly defineLazy*Getters methods when defining many lazy imports.

https://reviewboard.mozilla.org/r/165870/#review171682

Very nice! :-)

::: toolkit/components/extensions/extension-process-script.js:18
(Diff revision 2)
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> -XPCOMUtils.defineLazyModuleGetter(this, "MessageChannel",
> +/* globals ExtensionChild, ExtensionContent, ExtensionPageChild, MessageChannel */

Why is this needed?

::: toolkit/components/search/nsSearchService.js:28
(Diff revision 2)
> -XPCOMUtils.defineLazyModuleGetter(this, "clearTimeout",
> -  "resource://gre/modules/Timer.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "Lz4",
> -  "resource://gre/modules/lz4.js");
>  
>  XPCOMUtils.defineLazyServiceGetter(this, "gTextToSubURI",

definedLazyServiceGetters?

::: toolkit/components/telemetry/TelemetrySession.jsm:113
(Diff revision 2)
>  
>  var gLastMemoryPoll = null;
>  
>  var gWasDebuggerAttached = false;
>  
>  XPCOMUtils.defineLazyServiceGetter(this, "Telemetry",

defineLazyServiceGetters?

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8895145 [details]
Bug 1388215: Part 1 - Add defineLazyModuleGetters and defineLazyServiceGetters methods.

https://reviewboard.mozilla.org/r/166272/#review171674

::: js/xpconnect/loader/XPCOMUtils.jsm:351
(Diff revision 1)
> +   *        The object to define the lazy getter on.
> +   * @param aServices
> +   *        An object with a property for each service to be
> +   *        imported, where the property name is the name of the
> +   *        symbol to define, and the value is an array to be passed as
> +   *        additional arguments to defineLazyServiceGetter.

While it says the same thing, I think it would be more explict to say that the array contains first the contract id and then the interface name.

::: js/xpconnect/loader/XPCOMUtils.jsm:353
(Diff revision 1)
> +   *        An object with a property for each service to be
> +   *        imported, where the property name is the name of the
> +   *        symbol to define, and the value is an array to be passed as
> +   *        additional arguments to defineLazyServiceGetter.
> +   */
> +  defineLazyServiceGetters: function XPCU_defineLazyServiceGetters(

I would put this method right after defineLazyServiceGetter so that the 2 methods related to services and the 2 methods related to modules stay grouped.

::: js/xpconnect/loader/XPCOMUtils.jsm:359
(Diff revision 1)
> +                                   aObject, aServices)
> +  {
> +    for (let [name, service] of Object.entries(aServices)) {
> +      // Note: This is hot code, and cross-compartment array wrappers
> +      // are not JIT-friendly to destructuring or spread operators, so
> +      // we need to use indexed access instead.

Should a bug be filed on this? If so, I think this comment should include the bug number, or nobody will ever dare touching this in the future.

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8895146 [details]
Bug 1388215: Part 2 - Add eslint plugin support for defineLazy*Getters() methods.

https://reviewboard.mozilla.org/r/166274/#review171688

Comment 13

[Florian Quèze [:florian]]

Thanks for taking into account comment 2 and cleaning up so much of our code! :-)

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8895145 [details]
Bug 1388215: Part 1 - Add defineLazyModuleGetters and defineLazyServiceGetters methods.

https://reviewboard.mozilla.org/r/166272/#review171674

> I would put this method right after defineLazyServiceGetter so that the 2 methods related to services and the 2 methods related to modules stay grouped.

Fair enough. I was aiming to group all of the `define*Getters` methods, but it probably does make more sense to group them with the single getter methods that they call.

> Should a bug be filed on this? If so, I think this comment should include the bug number, or nobody will ever dare touching this in the future.

Probably not. After bug 1357862 lands, this will mostly cease to be an issue, except for chrome-to-content and extension-to-content access, and I doubt any JIT engineers will have time to spend on that kind of corner case.

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8894704 [details]
Bug 1388215: Part 3 - Use JIT-friendly defineLazy*Getters methods when defining many lazy imports.

https://reviewboard.mozilla.org/r/165870/#review171682

> Why is this needed?

Oops.

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e923e11fbb53b75bef851bd78dd9bb64ced246f
Bug 1388215: Part 1 - Add defineLazyModuleGetters and defineLazyServiceGetters methods. r=florian

https://hg.mozilla.org/integration/mozilla-inbound/rev/5294842bfd8315801ecf45dea0f3f16651d8a604
Bug 1388215: Part 2 - Add eslint plugin support for defineLazy*Getters() methods. r=florian

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9ab0caee26ba502d61c5a71bae819018ab6ac4
Bug 1388215: Part 3 - Use JIT-friendly defineLazy*Getters methods when defining many lazy imports. r=florian

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93925b6443b7
Fix broken browser.js after merge. r=bustage-fix on a CLOSED TREE

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e923e11fbb5
https://hg.mozilla.org/mozilla-central/rev/5294842bfd83
https://hg.mozilla.org/mozilla-central/rev/7b9ab0caee26
https://hg.mozilla.org/mozilla-central/rev/93925b6443b7