1601618 - Use XPCOMUtils.defineLazyModuleGetters() to defer loading of not immediately necessary modules

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now we make use of ChromeUtils.import() in most of our domains, even for modules which aren't necessary when loading the JSM, or creating the contained domain. This is especially expensive in content context.

As such we should move all of those imports over to XPCOMUtils.defineLazyModuleGetters() like the following:

const { XPCOMUtils } = ChromeUtils.import(
  "resource://gre/modules/XPCOMUtils.jsm"
);

XPCOMUtils.defineLazyModuleGetters(this, {
  OS: "resource://gre/modules/osfile.jsm",
});

Comment 1

[Andreas Tolfsen ❲:ato❳]

There has been talk about adopting ES module imports and/or making ChromeUtils.import lazy by default. I don’t know if there are concrete plans or what stage that work is at.

If memory serves me right the problem with implicit lazy loading (that ES modules do) is that there are plenty of cases elsewhere in Gecko that explicitly relies on the process-scoped behaviour of JSMs, and this may block us from adopting any one solution wholesale.

In any case, the modern way to lazy load modules is using ChromeUtils.defineModuleGetter, and it looks like XPCOMUtils.defineLazyModuleGetter was recently changed to use this internally. One caveat is that you must only use the three first arguments aObject, aName, aResource to gain this benefit. Otherwise it will revert to the old and slower JS-based import. I haven’t seen a single case where we rely on the custom behaviour offered by aSymbol, aPreLambda, aPostLambda, or aProxy. The plural version, XPCOMUtils.defineLazyModuleGetters uses ChromeUtils.defineLazyModuleGetter consistently.

To minimise the code we have to write we should prefer ChromeUtils.defineModuleGetter in cases with few imports and XPCOMUtils.defineLazyModuleGetters when there are more than a few, since importing XPCOMUtils.jsm adds a further three lines of boilerplate with its own import. I would argue we should avoid XPCOMUtils.defineLazyModuleGetter entirely because it’s easy to get the argument count wrong and because ChromeUtils.defineModuleGetter is functionally equivalent.

Comment 2

[Andreas Tolfsen ❲:ato❳]

I forgot to say that the downside with all the lazy loading APIs currently on hand is that they make importing multiple symbols from the same module rather awkward:

ChromeUtils.defineModuleGetter(
    this,
    "foo",
    "chrome://remote/content/Whatever.jsm"
);
ChromeUtils.defineModuleGetter(
    this,
    "bar",
    "chrome://remote/content/Whatever.jsm"
);

As compared to:

const { foo, bar } = ChromeUtils.import("chrome://remote/content/Whatever.jsm");

But hopefully the shared state ES module/JSM import problems I alluded to earlier will be resolved at some point and we can have pretty, standards compliant code again…

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

In those cases it would be good if our modules would just export a single namespace. Very good example is OS. Note that we should do as much as possible lazy loading of modules for JSWindowActor child instances. Otherwise memory usage will spike up, and performance will be degraded.

Comment 4

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #3)

In those cases it would be good if our modules would just export a single namespace. Very good example is OS.

Yes I agree, and that is the model we’ve largely followed throughout remote/. There have even been a couple of patches to move us further in that direction that you can see from the early history.

The notable exceptions are Sync.jsm and Error.jsm. We need to figure out how to structure the exported symbols in a meaningful way, so I’ve filed bug 1603167 to deal with this problem specifically.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

See bug 1660881 in how it was done in Marionette.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Not sure if we wanna do it for all the CDP specific modules, but at least we need it for BiDi and the shared modules.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1601618 - [remote] Use lazy loading of modules and delayed setting of global properties.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad4f4d69a6a9
[remote] Use lazy loading of modules and delayed setting of global properties. r=remote-protocol-reviewers,jdescottes

Comment 9

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ad4f4d69a6a9