1181967 - Destroy all lazy loader methods other than lazyRequire[Getter]

Status: NEW

(DevTools :: General, enhancement)

Description

[Jordan Santell [:jsantell] [@jsantell]]

On the loader, we have several lazy loading methods:

* lazyGetter
* lazyImporter
* lazyServiceGetter
* lazyRequireGetter

All of these can be replaced with `lazyRequireGetter` (and should be renamed to just lazyRequire at this point), and instead of having 4 lazy methods at the top of every file (and having to look up which one you want to use by looking at previous files), this just eliminates that confusion.

`lazyImporter` does almost the same thing, except only takes resource URIs, and has a different signature for destructing.

```
loader.lazyImporter(this, "ServicesModule", "resource://gre/modules/Services.jsm", "Services");

loader.lazyRequire(this, "Services", "resource://gre/modules/Services.jsm");
```
The 4th arg in lazyRequire deterines if arg 2 is the exports object itself, or one of its children. the `lazyImporter` scenario lets us rename whatever we're exposing, so maybe adding that to lazyRequire could be useful, but not that big of a deal IMO. Since loader can just load resource URIs, this should be fine. I haven't seen any uses of lazyImporter that use the 4th arg anyway.


lazyGetter has many use cases where it's a simple replacement for lazyRequire:
loader.lazyGetter(this, "EventEmitter", () => require("devtools/toolkit/event-emitter"));

But sometimes this does more than just load a module -- in that case, I think lazyRequire's 3rd argument can either be a module path/string, or a function that synchronously returns the unlazied object.

lazyServiceGetter, for the most part, can be fetched just through Services.jsm -- I don't think we need to get fancy for this, and I will never remember the inconsistent service names anyway.

I may be missing some use cases, but I think this removes a lot of complexity/confusion.

Comment 1

[Tom Tromey :tromey]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #0)

> lazyServiceGetter, for the most part, can be fetched just through
> Services.jsm -- I don't think we need to get fancy for this, and I will
> never remember the inconsistent service names anyway.
> 
> I may be missing some use cases, but I think this removes a lot of
> complexity/confusion.

One nice thing about lazyServiceGetter is that it could provide a way
to mock services for simpler testing.  See bug 1181268.  It would be
nice, if we dropped lazyServiceGetter, to have some other way to mock
services.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Overall, I agree, less redundant paths to do the same thing is better.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #0)
> lazyServiceGetter, for the most part, can be fetched just through
> Services.jsm -- I don't think we need to get fancy for this, and I will
> never remember the inconsistent service names anyway.

I am not sure what you are purposing here... you want to remove it?  I think it's nice to offer a path to loading all the "usual" things we need to load via the loader.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Once lazyRequire also handles functions as a third argument, it becomes a combo of lazyRequireGetter and lazyGetter. With that, importing services becomes:

loader.lazyRequire(this, "name", ()=>Cc[...].getService(...))
Instead of
Loader.lazyServiceGetter(this, "name", "@mozilla..", "nsI...")

So it still uses the same one function. In the codebase, lazyServiceGetter is seldom used in favor of lazyGetter, anyway, making this just a name switch in that case.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Not sure of a good name that handles module loading of js/jsm as well as an arbitrary function. "LazyRequire" makes it sound like its always an external source, but could just be a lazy computation.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #3)
> Once lazyRequire also handles functions as a third argument, it becomes a
> combo of lazyRequireGetter and lazyGetter. With that, importing services
> becomes:
> 
> loader.lazyRequire(this, "name", ()=>Cc[...].getService(...))
> Instead of
> Loader.lazyServiceGetter(this, "name", "@mozilla..", "nsI...")

Oh, hmm...  I guess I did not realize what you were suggesting.

Why do you want something called "lazyRequire" that does not actually call "require"?  Isn't that confusing?

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Notes from changing this:


Can’t rename exposed properties when destructuring, which you previously could do with JSMs. This is only really common in Promise.jsm, which should not be lazily loaded anyway.
lazyImporter(this, “promise”, “path/to/promise”, “Promise”);
lazyRequire(this, “promise”, () => require(“path/to/promise”).Promise);

lazyServiceGetter is used, and replaced, via:
lazyServiceGetter(this, “myServiceName”, “@mozilla/service;1”, “nsIInterface”);
lazyRequire(this, “myServiceName”, () => Cc[“@mozilla/service;1”].getService(Ci.nsIInterface));
This is very rarely used anyway — usually it just uses lazyGetter, which is the same API as the remaining method once it accepts functions instead of module paths, like this pattern used in many places:

lazyGetter(this, “fn”, () => require(“path/to/module”).fn);
Which is now:
lazyRequire(this, “fn”, “path/to/module”, true)

lastImporter automatically destructures JSMs. Now must add the extra boolean for restructuring.
lazyImporter(this, “gDevTools”, “path/to/gdevtools”);
lazyRequire(this, “gDevTools”, “path/to/gdevtools”, true);

Some areas use the XPCOMUtils methods directly — some i changed, and some didn’t have the loader or too many, so just left it there in other cases. Main thing was so that we do not use these direct loader methods, other than just one.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

I just want one lazy importer -- the arbitrary function case makes things a bit different, and maybe that should remain a different function, but it's nice to just swap out a module path for a function that does whatever you need to -- maybe more of a naming issue

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

We discussed on IRC, perhaps a name like "lazyDefine"...  As long as we do not have something using the word "require" which only sometimes calls "require", that was my main objection.

Also service handling should probably remain separate.  It sounds too magical to lump that into one function.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1181967-lazy-define.patch

So this is with lazyDefine, which handles the previous lazyRequireGetter and lazyGetter scenarios. Not completely sold on the name. Some things aren't working yet right now but looking into it

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1181967-lazy-define.patch

Some fixes  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d69a5e853bf

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1181967-lazy-define.patch

moorrreeeeeee
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=6838146c5af6

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8631859 [details] [diff] [review]
1181967-lazy-define.patch

Review of attachment 8631859 [details] [diff] [review]:
-----------------------------------------------------------------

So far I only looked at a random sample of files.

How are you feeling about the name?  Does it feel clear that "lazyDefine" loads things?  Is that what you expect something called "lazyDefine" to do?

I will review the full set when you eventually request r?.

::: browser/devtools/animationinspector/animation-controller.js
@@ +18,1 @@
>                                   "devtools/toolkit/event-emitter");

Nit: Fix indent

@@ +20,1 @@
>                                   "devtools/server/actors/animation", true);

Nit: Fix indent

::: browser/devtools/debugger/debugger-commands.js
@@ +6,5 @@
>  
>  const { Cc, Ci, Cu } = require("chrome");
>  const l10n = require("gcli/l10n");
>  
> +loader.lazyDefine(this, "gDevTools", "resource:///modules/devtools/gDevTools.jsm");

I think we need to pre-load this file in the loader...  It's meant to be loaded once per browser session (which is what Cu.import does), but by loading via require, it's now one per DevTools loader, and you can have multiple loaders per browser session.

Mentioned this to jlongster today too.

::: browser/devtools/framework/toolbox-options.js
@@ +6,5 @@
>  
>  const {Cu, Cc, Ci} = require("chrome");
>  const Services = require("Services");
>  const promise = require("promise");
> +const { devtools: loader } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

If we have require, don't we have the loader too?

::: toolkit/devtools/Loader.jsm
@@ +320,1 @@
>     * Define a getter property on the given object that requires the given

Rewrite this, does not always call |require|.

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

I think maybe should start a brief discussion on the mailing list. Must be a better name. 

We should move gDevTools as a built in module, yeah.. Fwiw it uses Cu.import under the hood, so its still the same instance, but just the loader wrapper around it gets recreated once per loader

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1181967-lazy-define.patch

Updated with jryans' comments, and try fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=edac2c1022da

w/r/t gDevTools being a part of loader and loaded once -- this behaviour isn't any different than what we have currently, and I'd like to avoid additional complexity in this painful process, as it's already 100 files large

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #14)
> w/r/t gDevTools being a part of loader and loaded once -- this behaviour
> isn't any different than what we have currently, and I'd like to avoid
> additional complexity in this painful process, as it's already 100 files
> large

Since you pointed out we just Cu.import in the loader, I agree we don't need special behavior for gDevTools.  As long as there's only one instance, it's okay, and it seems that's what we'll have.

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

lazyDefine is not a bad name: it seems intuitive enough and it also bears a resemblance to AMD-style module definition - define("foo", ["bar"], function() {...}) - for those who are familiar with it.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Panos Astithas [:past] from comment #16)
> lazyDefine is not a bad name: it seems intuitive enough and it also bears a
> resemblance to AMD-style module definition - define("foo", ["bar"],
> function() {...}) - for those who are familiar with it.

One quirk is that AMD define()'s focus is on "defining" the current module (the name in the first arg), while here lazyDefine() is about importing modules you require, which for AMD is the array in the second arg and not really what the word "define" seems to mean.

Anyway, lazyDefine might still be a fine name here, as long it's clear wording for "loading / importing / requiring" a dependency.

Comment 18

[Jordan Santell [:jsantell] [@jsantell]]

Not able to get to this for a bit.

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@jsantell: What is the status of this? I mean, lazyDefine is a decent name and I assume the patch works?

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

If possible I'd like to complete my file move (bug 912121) before landing something here.

I have various scripts based around the current names that rewrite the tree, so I'd like to keep that as-is until the file move is done.

Comment 21

[Jordan Santell [:jsantell] [@jsantell]]

It was working before, probably just needs to be updated for bitrot and bug 912121

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I'll take this.

Comment 23

[Matteo Ferretti [:zer0] [:matteo]]

My two cents about the APIs for this. During a refactoring we had, we transform this:

const {
  isAnonymous,
  isNativeAnonymous,
  isXBLAnonymous,
  isShadowAnonymous,
  getFrameElement,
  loadSheet
} = require("devtools/shared/layout/utils");

into this:

loader.lazyRequireGetter(this, "isAnonymous", "devtools/shared/layout/utils", true);
loader.lazyRequireGetter(this, "isNativeAnonymous", "devtools/shared/layout/utils", true);
loader.lazyRequireGetter(this, "isXBLAnonymous", "devtools/shared/layout/utils", true);
loader.lazyRequireGetter(this, "isShadowAnonymous", "devtools/shared/layout/utils", true);
loader.lazyRequireGetter(this, "getFrameElement", "devtools/shared/layout/utils", true);
loader.lazyRequireGetter(this, "loadSheet", "devtools/shared/layout/utils", true);

It's much too verbose, and it's hard to understand at glance. 
We should at least have the capability to mimic the destructuring, something like:

```js
loader.lazyDefine(this, [
  "isAnonymous",
  "isNativeAnonymous",
  "isXBLAnonymous",
  "isShadowAnonymous",
  "getFrameElement",
], "devtools/shared/layout/utils");
```

Since it's an array, there is no flag for destructuring, it's implicit. It would avoid repetition, and it closest to the non-lazy syntax. We could also pass an object, for destructuring and renaming:

```js
loader.lazyDefine(this, [
  {"isAnonymous": "isAnon"},
  "isNativeAnonymous",
  "isXBLAnonymous",
  "isShadowAnonymous",
  "getFrameElement",
], "devtools/shared/layout/utils");
```

I also don't like too much the name; I would prefer one word for such things, if we also have already `loader` as prefix it might be sufficient. Maybe `loader.lazy(…)` is too minimalistic, but it's already clear to me that we're "loading in a lazy way" something.

Also, does anyone has numbers about how slow would be an implementation with `Proxy`? I'm just curious, on paper it seems a perfect fit for it, but I'm worried about performance.