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

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

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

Updated

a year ago
Blocks: 1363905
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
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, [
  ...
]);

?
(Assignee)

Comment 4

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

Comment 5

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

Updated

a year ago
Component: WebExtensions: General → General
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

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

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

Comment 11

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

Comment 12

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

https://reviewboard.mozilla.org/r/166274/#review171688
Attachment #8895146 - Flags: review?(florian) → review+
Thanks for taking into account comment 2 and cleaning up so much of our code! :-)
(Assignee)

Comment 14

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

Comment 15

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

Comment 16

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

a year ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93925b6443b7
Fix broken browser.js after merge. r=bustage-fix on a CLOSED TREE
Duplicate of this bug: 1325373

Comment 19

a year ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.