Closed Bug 1432901 Opened 6 years ago Closed 2 years ago

Prototype loading ES6 Module as JSM

Categories

(Core :: XPConnect, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
102 Branch
Tracking Status
firefox102 --- fixed

People

(Reporter: tcampbell, Assigned: jonco)

References

(Depends on 1 open bug, Blocks 4 open bugs)

Details

Attachments

(17 files, 5 obsolete files)

24.95 KB, patch
Details | Diff | Splinter Review
16.65 KB, patch
Details | Diff | Splinter Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
I'd like to prototype a modified JSM loader that can use ES6 modules in place of JSMs.

Rough scope:
- Replace EXPORTED_SYMBOLS with 'export' keywords
- Don't support 'import' keyword yet (continue to use Cu.import or whatever as usual)
- The loader would create a global (or NonSyntacticVariablesObject on the shared JSM global), import the module script into it, and then expose the exported symbols in the same way EXPORTED_SYMBOLS does

One breaking change will be that ES6 modules don't have a global 'this' value, so the |this.Foo = function() { ... }| pattern from B2G should be converted to |function Foo() { ... }| like normal JS. :kmag has a hacky tool to rewrite our existing JSMs to do this sort of transform for similar purposes. We may be able to use this to mass convert JSMs to ES6 modules if this experiment sticks.

We would use this custom loader for now to import the module, and it would continue to load a single copy with shared global state so it works in place of JSMs.

The module has a ModuleNamespaceObject which we can probably use in place of EXPORTED_SYMBOLS.

The hope of this effort is to replace all JSMs with ES6 modules and be less weird. Those modules would still be loaded into some sort of shared JSM global so that the existing shared-global-state behavior works.
Priority: -- → P3
Here is initial design for my prototype. This is more about flushing out
problems than a final design, but feedback is welcome.

Add ChromeUtils.importJSM / ChromeUtils.importES6
    - These APIs load JSM or ES6 modules, returning the set of exported
      bindings.
    - These can be used for any code context, including ES6 modules where
      |this| is |undefined|.
    - Using special return objects (probably JSAPI resolve hooks), the
      module will (optionally) be lazily loaded only when bindings are
      first accessed. This will be used to replace
      ChromeUtils.defineModuleGetter uses.

Prerequisites to convert module to ES6
    - All current imports must be replaced by ChromeUtils.importJSM
    - The module must not use global |this| (implicitly or explicitly)
	- Replace ChromeUtils.import with ChromeUtils.importJSM
	- Replace ChromeUtils.defineModuleGetter with ChromeUtils.importJSM
	- Replace |this.foo = ...| with |var foo = ...|
    - Problem areas
	- XPCUtils.defineLazyGetter needs an alternative
	- Some code accesses bindings that are not explicitly exported
    - Rename file to *.mjs (what Node uses)
    - Replace EXPORTED_SYMBOLS with export keywords instead
    - Update all importers to use ChromeUtils.importES6

Binding List Issues
    - By using a resolve hook, we can generally mitigate the impact to the
      SpiderMonkey JIT and have fast access to the bindings of the target
      module.
    - A proper ES6 Module Namespace Exotic Object works similar to a Proxy in
      that mutations in the target module are reflected in the importers
      namespace object. This is annoying for the JITs.
    - If the current JSM import destructures the return value of an import, it
      will see a copy of the current binding values.
    - If the current JSM import uses a target object (or implicit global this),
      the binding values will be cached from when the module was first loaded
      (possibly by someone else). The exception to this is if any of the names
      in EXPORTED_SYMBOLS was not presently defined in which case we return a
      snapshot but don't save the list in a cache yet. The next person to
      import the JSM may get a more current value.
    - Ideally, our module code should be written so these subtleties don't come
      up.
    - Ideally, ES6 modules should be able to use normal import syntax to load
      further modules. Lazy loading will still require using the
      ChromeUtils.importJSM API.
Assignee: nobody → tcampbell
For background, I've been trying to add Typescript annotations to webextensions modules (JSMs currently), and the biggest hurdle is the difference between JSM/commonJS(node)/es6 module semantics.

So I'm eager to try this out, and convert the first chunk of firefox code to es6 modules.  So from that point of view, I have "opinions" ;)

>     - Using special return objects (probably JSAPI resolve hooks), the
>       module will (optionally) be lazily loaded only when bindings are
>       first accessed. This will be used to replace
>       ChromeUtils.defineModuleGetter uses.

>     - Problem areas
> 	- XPCUtils.defineLazyGetter needs an alternative

I suggest we stick with semantic/syntax that could be expressed in other engines (and that Typescript will understand).  So javascript that looks like either of these options:

    const ExampleModule = ChromeUtils.importES6lazy(".../ExampleModule.mjs");

or

    const LAZY = {
      get ExampleModule() {
        return ChromeUtils.importES6(".../ExampleModule.mjs");
      },
      get AnotherExample() {
        // ...
      },
    };

    ChromeUtils.memoizeLazyGetters(LAZY);


The second would obviously mean using LAZY.ExampleModule instead of a global, but I think our code being more explicit would be good overall, and it could be generalized as a pattern for all other things we try to do lazily (creating xpcom services, accessing prefs branches, etc).
Depends on: 1469004, 1308252
Depends on: 1514594
Depends on: 1517952
Depends on: 1524027

Add support for ChromeUtils.import to load files with a .mjs extension as
EcmaScript Modules instead of as JSMs. They continue to be loaded in the
current system component global.

NOTE: We use an '.mjs.jsm' extension for now since parts of the browser still
seem to care about the .jsm extension and I get test failures.

To convert a JSM to a ES-Module perform the following steps:

  • Rename the file to .mjs.jsm and update references
  • Remove EXPORTED_SYMBOLS and use 'export' keyword
  • Remove top-level uses of 'this' as it is defined as 'undefined' in ES
    Modules

NOTE: Using 'import' keyword is untested, but ChromeUtils.import can still be
used within ES Modules.

Depends on D52763

Here is a new attempt at prototyping thing. It needs more testing, but it seems to work a lot better than I expected.

Using a '.mjs' extension was giving me some weird failures, so I'm using '.mjs.jsm' for now and pattern matching inside of ChromeUtils::Import. I haven't tested exotic cases with different globals yet. The keyword import wasn't immediately working, but that is largely due to import path resolution rules I need to investigate.

Attachment #9108267 - Attachment description: Bug 1432901 - (WIP) Convert BrowserTabParent to ES Module → Bug 1432901 - (WIP) Convert some JS Window Actors to ES Modules

It seems like the '.mjs' failure was just a mistake on my part. I've fixed the file extension back to the simple '.mjs'. I've also updated the sample patch to convert all the trivial actors to get more testing.

One thing to note is there is no startup cache support for any of this yet, but there is active work related to Bug 1308252.

(In reply to Ted Campbell [:tcampbell] from comment #6)

It seems like the '.mjs' failure was just a mistake on my part. I've fixed the file extension back to the simple '.mjs'. I've also updated the sample patch to convert all the trivial actors to get more testing.

One thing to note is there is no startup cache support for any of this yet, but there is active work related to Bug 1308252.

Thanks Ted! That looks great, and the actor patch is impressively small. Are tests green with that? Once we have good enough perf (at least startup cache), I think landing incremental patches like this separately makes a lot of sense and is something we could break down easily - assuming that we could import a jsm from an mjs such that we don't have cyclical import issues making it so we need to do everything at once.

For the the actor patch, did you manually make those EXPORTED_SYMBOLS -> export changes did you use some kind of script? We should probably make an attempt to migrate a large number of jsm's at once at least locally so that we can run talos to confirm that we aren't going to get a series of small / hard to detect perf regressions if we do it in small batches.

Flags: needinfo?(tcampbell)

Maybe the .mjs problem is related to bug 1589895?

Just to note, we'll need to update ESLint's settings for the new mjs files. This shouldn't be too complicated, mainly making sure that mjs are seen as modules, and applying the rules/environments that we have for jsm files to mjs as well.

I'm quite happy to help out there at an appropriate time.

(In reply to Tom Schuster [:evilpie] from comment #8)

Maybe the .mjs problem is related to bug 1589895?

I doubt it. I think the mjs extension is working fine actually. I just confused myself trying to test things.

(In reply to Brian Grinstead [:bgrins] from comment #7)

For the the actor patch, did you manually make those EXPORTED_SYMBOLS -> export changes did you use some kind of script? We should probably make an attempt to migrate a large number of jsm's at once at least locally so that we can run talos to confirm that we aren't going to get a series of small / hard to detect perf regressions if we do it in small batches.

This is manually converted right now. I believe :kmag has scripts and ideas for mass conversion though.

Basic mochitests are passing but I'll add a few more now. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e33e9f3c71a1905af2100ebdc39815adb200e566

Performance Concerns I'd like to see checked before landing things:

  • Is there perf impact accessing properties of returned module (module namespace proxy) vs the previous plain-object?
  • Is the perf of accessing global properties such as 'undefined' actually improved?
  • Does startup cache work?
  • Does preload/prefetch cache work?

Open Questions

  • What is best design for defineLazyModuleGetter replacement?
    • Currently uses a getter on the global that gets redefined to a property. This clobbers some JIT metadata today.
    • Might be possible to enhance ModuleNamespaceObject to do the lazy load. This should be compatible with current JIT optimizations for it.
Flags: needinfo?(tcampbell)

The basic problem with converting a large number of JSMs at once is that we need to decide how to handle lazy getters, and a large subset of JSMs currently use lazy module getters.

That said, I can write scripts that detect the compatible cases and mass convert all of those.

(In reply to Ted Campbell [:tcampbell] from comment #11)

Performance Concerns I'd like to see checked before landing things:
...

  • Is the perf of accessing global properties such as 'undefined' actually improved?

Well, in any case, it can hardly be made worse.

(In reply to Ted Campbell [:tcampbell] from comment #11)

Open Questions

  • What is best design for defineLazyModuleGetter replacement?
    • Currently uses a getter on the global that gets redefined to a property. This clobbers some JIT metadata today.
    • Might be possible to enhance ModuleNamespaceObject to do the lazy load. This should be compatible with current JIT optimizations for it.

Is it technically feasible to make them work more or less exactly how we currently use them (support synchronous loading of mjs files at arbitrary times)? I know it's not ideal, and we may lose some JIT benefits, but IIUC it won't leave us in a worse state then we are today. And if we could split the project up into phases like:

  1. Support the same loading semantics as JSMs.
  2. Do a mass-migration (at least partially automated) where we rename jsm->mjs and rewrite exports.
  3. Improve (or stop) lazy loading so we can get JIT wins.

We'd be getting nice wins each step of the way. It also would give us a good target before landing (2) to test performance against the baseline.

(In reply to Brian Grinstead [:bgrins] from comment #13)

Is it technically feasible to make them work more or less exactly how we currently use them (support synchronous loading of mjs files at arbitrary times)? I know it's not ideal, and we may lose some JIT benefits, but IIUC it won't leave us in a worse state then we are today. And if we could split the project up into phases like:

The synchronous loading on-demand should be fine. The issue right now is that they access global this which doesn't exist in .mjs modules. What I'm wondering about is if the value returned from ChomeUtils.import('..mjs') could be automatically lazy. It's type is js::ModuleNamespaceObject but we could add a lazy mode there without impacting JIT support.

let lazyFoo = ChromeUtils.import('foo.mjs');

 function doCoolThings()
{
   lazyFoo.CoolAPI(); // Trigger the module load here.
}

(In reply to Ted Campbell [:tcampbell] from comment #14)

(In reply to Brian Grinstead [:bgrins] from comment #13)

Is it technically feasible to make them work more or less exactly how we currently use them (support synchronous loading of mjs files at arbitrary times)? I know it's not ideal, and we may lose some JIT benefits, but IIUC it won't leave us in a worse state then we are today. And if we could split the project up into phases like:

The synchronous loading on-demand should be fine. The issue right now is that they access global this which doesn't exist in .mjs modules. What I'm wondering about is if the value returned from ChomeUtils.import('..mjs') could be automatically lazy. It's type is js::ModuleNamespaceObject but we could add a lazy mode there without impacting JIT support.

let lazyFoo = ChromeUtils.import('foo.mjs');

 function doCoolThings()
{
   lazyFoo.CoolAPI(); // Trigger the module load here.
}

Oh, that sounds really nice. Seems like if we had that we could rewrite JS that currently uses lazy getters to just eagerly import at the top, but get the same behavior / performance as the lazy getters. Maybe this should be opt in to prevent confusion & diversion from the specified module behavior (ChromeUtils.lazyImport('foo.mjs');?). Although I do suspect we'd see perf wins if we changed it globally from modules that could be lazily loaded but aren't currently.

There are implications to the ergonomics, although most of this applies to existing JSM usage already.

// Content ES Module
import { nameA, nameB } from "foo.mjs";

// Non-Module
let { nameA, nameB } = ChromeUtil.import("foo.mjs");

// Wildcard Import
import * as fooMod from "foo.mjs";

// Hypothetical lazy chrome import
let fooMod = ChromeUtil.import("foo.mjs");

// Dynamic import
import("foo.mjs").then(fooMod => { ... })

In the future, if we use 'import' instead of ChromeUtils, we will still need a non-standard way to replicating this lazy behaviour. For example, we could add an 'importLazy' method to the privileged global that returns the on-demand ModuleNamespaceObject instead of a Promise<ModuleNamespaceObject> that standard dynamic import returns.

In terms of script-based rewriting, :k88hudson's work on https://github.com/k88hudson/babel-plugin-jsm-to-esmodules may offer useful food for thought (or even code re-use?).

Depends on: 1602931

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #17)

In terms of script-based rewriting, :k88hudson's work on https://github.com/k88hudson/babel-plugin-jsm-to-esmodules may offer useful food for thought (or even code re-use?).

Looks like that is the inverse of something I played around with: https://github.com/Mossop/babel-plugin-transform-es2015-modules-mozilla

This is an updated version of the main patch that includes support for the StartupCache. It seems to function OK on smoke-tests, but I ran into a crash well into browsing that may be related. It's a bit weird because the crash itself is an ASSERT failure about an HTTP thread trying to schedule an idle task on the Main thread and failing some sanity checks on which threads are allowed to schedule which jobs where - nothing to do with the underlying changes to the actual cache.

Still investigating that, and still need to add support for the preload cache, which is important for performance.

Rebased version of patch that actually changes a few modules to use ES6 instead of JSM behaviour.

Rebased patch that applies to current tip (10059adf0045).

Attachment #9120517 - Attachment is obsolete: true
See Also: → 1525652
Blocks: 1525652
See Also: 1525652
See Also: → 1525652
Assignee: tcampbell → ystartsev
Attachment #9205344 - Attachment description: Bug 1432901 - (WIP - updated prototype) Support ES Modules in ChromeUtils.import → WIP: Bug 1432901 - (WIP - updated prototype) Support ES Modules in ChromeUtils.import

I have something that is getting close to being finished, quick update on it:

You will be able to do relative imports now, the same pattern that is used in regular ES6 module imports. You can see this in action here: https://phabricator.services.mozilla.com/D113521

A couple of things I noticed while transitioning certdecoder to the new "module" form.

  1. our reliance on requirejs will make some migrations quite frustrating. We might want a pattern that can be broadly applied to allow this. I used rollup to create requirejs like bundles in this case. However, since it was exporting for node's es6 modules, we probably want a different setting than what i used.

  2. We may want to not use .mjs -- it may make incremental transitions more difficult. At least, this was my experience when moving cert decoder. What might be the case is it ensures that we transition coherent chunks. That will probably mean we want to have good error messages. The other alternative is -- rather than Cu.import, we create a new funtion Cu.importModule.

2a) If we use mjs, any dependencies that we import from a vendor will also have to be renamed to be .mjs. This could potentially be a huge pain. Jar files also make this a bit difficult, so we might want to say that we adopt a consistent way of loading libraries (such as asn1js & co in the example implementation)

2b) that said, mjs is kind of nice because we don't have any other signal that something is a script import

Attachment #9108266 - Attachment is obsolete: true
Attachment #9108267 - Attachment is obsolete: true

Applied StartupCache/ScriptPreloader cache to ES6 modules loaded by mozJSComponentLoader::ObjectForLocation*
https://treeherder.mozilla.org/jobs?repo=try&revision=1fdf85abc2e6d73a1989b56e44f235f9a73b1b2a

The change is mostly straightforward. it uses the same caching mechanism as what current JSM uses for regular script stencil,
except that it uses different cachePath than regular script (currently it's just informational. no requirement applied yet)

Depends on: 1759881

taking over.

Assignee: ystartsev → arai.unmht

import part will be handled in bug 1311728

Assignee: arai.unmht → nobody
Depends on: 1311728
Assignee: nobody → jcoppeard
Depends on: 1767800
Depends on: 1767829

I've got some patches (which I'll post shortly) that implement a ChromeUtils.importModule method to loads an ES6 module using the module loader framework.

This turned out to be complicated for several reasons and required a few workarounds, so I'll explain these below:

  1. ChromeUtils.importModule is synchronous, whereas the module loader is more event-driven/asynchonrous.

The module loader uses promises internally, both DOM promises and mozJSPromise.

The former happens because the JS API to evaluate a module supports top-level await and hence returns a promise. The patches add an option to get an immediate response when evaluating a module rather than waiting for the promise reactions to run. This is necessary so the new method can to throw an exception when an import fails.

The latter is because the module loader uses mozPromise internally to e.g. wait for module imports to be loaded. The patches work around this by allowing a derived module loader to supply its own event target and creating a trival SyncEventTarget that dispatches runnables by executing them immediately.

  1. The module loader API doesn't fit well with how mozJSComponentLoader works.

Firstly, the loader base class lets the derived class implement two operations in particular: fetch module source and compile module source. Currently a vector is used to hold source text or bytecode. However mozJSComponentLoader may use a memory mapped file to access the source, which isn't easy to integrate into this model.

Secondly, mozJSComponentLoader can cache loaded scripts. This relies on having cache key and other contextual information available from fetching available until after compilation when the cache is written to. This could be achieved by storing this information in the load context, but the only purpose this serves is to make mozJSComponentLoader to conform to the module loader model. It has no real gain and only makes the code more confusing.

The patches avoid this and implement both fetch and compile in the fetch step, storing the results in the load context. This is not ideal but requires fewer changes to mozJSComponentLoader.

Comments welcome on this approach. In future we should revisit the loader model model and hopefully find a way to generalise it. Since derived loaders end up having quite different implementations I think it could most sense to combine both these steps into a single operation to get a compiled module, i.e. punt everything to the derived class.

  1. AutoJSAPI steals and reports exceptions, but you don't always want that.

The module loader uses AutoJSAPI and AutoEntryScript internally, but these automatically 'report' errors which removes the from the context. We don't want that here so the patches add a way to evaluate a module in a given context.

  1. mozJSComponentLoader is not cycle collected and is destroyed after the cycle collector during shutdown.

One final nit. If anything reacable from mozJSComponentLoader (e.g. the new module loader) participates in a cycle then it won't get cleaned up and will leak. This required adding a method to the module loader to manually break any cycles. This is called before the cycle collector shuts down.

For the module loader, since it was made with the Worker/DOM first, I think we can change it however you see fit in order to make this work. Maybe that means taking stuff out, or possibly overriding the functions in question. Since it looks like StartOrRestartModule is the issue.. looking forward to seeing the patches.

It's a bit annoying that we have to convert between the module record object
and module script like this. I filed bug 1766810 to change the APIs but it's a
resonable amount of work at this point.

This adds JS APIs to get the module object for a module script and get a module
namespace object. The former required some refactoring of JSScript::module
(which could return null) into isModule() and module().

As explained in the bug, this adds the option to throw errors immediately
rather than by rejecting a promise. The promise is always settled at this point
since we don't allow top-level await.

Depends on D145554

Since mozJSComponentLoader is destroyed after the cycle collector, we need to
break cycles manually before the final cycle collection otherwise we get a
memory leak. Unload() is called at an appropriate earlier point in shutdown.

Depends on D145566

This adds the parameter to the module loaders evaluation method. I also
rewrote the comments a bit to make this section clearer based on my
understanding of how this works.

This is called ComponentModuleLoader rather than ModuleLoader as memory leak
detection doesn't like it if we have more than one class with the same name,
even if they're in different namespaces. Other name suggestions welcome.

As explained in the bug, we need to dispatch without using the event loop so we
can complete the import synchronously.

This will be used to hold compilation results before they are passed to the base class.

This adds a flag to ComponentLoaderInfo to say whether it represents a module
and calls the appriate JS APIs based on this.

This is the main patch that finally implements the import.

The derived module loader uses a list of pending load requests to drive the system.

Attachment #9275167 - Attachment description: Bug 1432901 - Part 13: Add some tests for ChromeUtils.importModule r?yulia → Bug 1432901 - Part 14: Add some tests for ChromeUtils.importModule r?yulia
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92d99a38d1c5
Part 1: Add module APIs to get the module object for a script and the namespace object for a module r=yulia
https://hg.mozilla.org/integration/autoland/rev/27ad92991bf6
Part 2: Add an option for ModuleEvalute to throw errors synchronously r=yulia
https://hg.mozilla.org/integration/autoland/rev/d45158394767
Part 3: Add option for synchronous module evaluation in the module loader r=yulia
https://hg.mozilla.org/integration/autoland/rev/83f5315e9774
Part 4: Add a module loader class skeleton for mozJSComponentLoader to use r=yulia
https://hg.mozilla.org/integration/autoland/rev/5e1e77e463a1
Part 5: Give BackstagePass a module loader r=yulia
https://hg.mozilla.org/integration/autoland/rev/4e3fbb8e9502
Part 6: Give our new module loader an event target that dispatches synchronously r=yulia
https://hg.mozilla.org/integration/autoland/rev/e8b8b8c6454c
Part 7: Add a load context for use by the new module loader r=yulia
https://hg.mozilla.org/integration/autoland/rev/d0348e9694a3
Part 8: Add option to compile source to module stencil in mozJSComponentLoader r=yulia
https://hg.mozilla.org/integration/autoland/rev/aa07b7d5d51a
Part 9: Add a method to mozJSComponentLoader to load a single module r=yulia
https://hg.mozilla.org/integration/autoland/rev/844364dcbd75
Part 10: Implement mozJSComponentLoader ImportModule method to synchronously import an ES6 module r=yulia
https://hg.mozilla.org/integration/autoland/rev/c7d5d9884fc4
Part 11: Add ChromeUtils.importModule method to import and ES6 module r=smaug
https://hg.mozilla.org/integration/autoland/rev/1a76019fc184
Part 12: Break cycles manually during shutdown r=yulia
https://hg.mozilla.org/integration/autoland/rev/0684b7a01ac6
Part 13: Re-enable compile option to disallow top-level await r=yulia
https://hg.mozilla.org/integration/autoland/rev/217b34687aa2
Part 14: Add some tests for ChromeUtils.importModule r=yulia
No longer depends on: 1602931

Comment on attachment 9218653 [details]
WIP: Bug 1432901 - (WIP) test implementation of certdecoder in network helper with Cu.import

Revision D113521 was moved to bug 1768816. Setting attachment 9218653 [details] to obsolete.

Attachment #9218653 - Attachment is obsolete: true
Regressions: 1768871
Attachment #9205344 - Attachment is obsolete: true
No longer regressions: 1768871
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: