Closed Bug 1499140 Opened Last year Closed Last year

Implement dynamic module import in the shell

Categories

(Core :: JavaScript Engine, enhancement)

61 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(4 files, 1 obsolete file)

As a step towards bug 1342012, implement dynamic module import in the shell.
Patch to implement the framework for dynamic import:
 - add a new embedding hook for dynamic import and public APIs to get/set it
 - add an API to finish a dynamic import, to be called by the hook implementation
 - add a bytecode JSOP_DYNAMIC_IMPORT and compile this if the hook has been set
 - implement this in the interpreter

The implementation does the things described in https://tc39.github.io/proposal-dynamic-import/#sec-import-calls - mostly setting up a promise and calling the embedding hook to load the module.

I factored out RejectPromiseWithPendingError from Stream.cpp as this was useful when dealing with promises.
Attachment #9017551 - Flags: review?(jdemooij)
This is my attempt to compile JSOP_DYNAMIC_IMPORT in the JITs.  It just compiles a VM call.

This works on linux64 but fails on arm builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=281ef239bdcda2bdc6edd2ee1143e925a1510998&group_state=expanded&selectedJob=205830449

Can you tell me what I did wrong?  Something about values being two words on 32-bit?
Attachment #9017557 - Flags: feedback?(jdemooij)
Finally, implement this in the shell.  Most of this is adding shell functions for getting/setting the import hook and call the JS APIs.  There's a separate function for aborting a dynamic import so that we can pass an error object (which is eventually used to reject the promise).  In the JS API this would happen by calling FinishDynamicModuleImport with a pending exception.

The implementation in the module loader is pretty straightforward: resolve the path, load the module and call the finish API.  A real implementation would be asynchronous, whereas this is not.

One thing is we need to tell the shell module loader about classic script paths so that relative imports work.  The patch fakes this a bit by creating the same kind of object that the loader uses in RegisterScriptPathWithModuleLoader.  I suppose it would be better to have another hook for calling into the loader to tell it about these scripts but it doesn't seem that important.  I can do this if you like though.
Attachment #9017562 - Flags: review?(jdemooij)
Comment on attachment 9017551 [details] [diff] [review]
bug1342012-implement-dyanmic-import

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

::: js/src/builtin/ModuleObject.cpp
@@ +1816,5 @@
> +        return nullptr;
> +    }
> +
> +    RootedObject result(cx, moduleResolveHook(cx, referencingPrivate, specifier));
> +    if (!result)

Nit: {}, and a few times below

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8878,5 @@
>          break;
>  
>        case ParseNodeKind::CallImport:
> +        if (!cx->runtime()->moduleDynamicImportHook) {
> +            reportError(nullptr, JSMSG_NO_DYNAMIC_IMPORT);

Should we change this message to something saying the hook isn't installed? Or just MOZ_CRASH with a message saying the embedder has to install the hook?

@@ +8881,5 @@
> +        if (!cx->runtime()->moduleDynamicImportHook) {
> +            reportError(nullptr, JSMSG_NO_DYNAMIC_IMPORT);
> +            return false;
> +        }
> +        if (!emitTree(pn->as<BinaryNode>().right()) || !emit1(JSOP_DYNAMIC_IMPORT))

Nit: {}

::: js/src/js.msg
@@ +614,5 @@
>  MSG_DEF(JSMSG_MISSING_NAMESPACE_EXPORT,  0, JSEXN_SYNTAXERR, "export not found for namespace")
>  MSG_DEF(JSMSG_MISSING_EXPORT,            1, JSEXN_SYNTAXERR, "local binding for export '{0}' not found")
>  MSG_DEF(JSMSG_BAD_MODULE_STATUS,         0, JSEXN_INTERNALERR, "module record has unexpected status")
>  MSG_DEF(JSMSG_NO_DYNAMIC_IMPORT,         0, JSEXN_SYNTAXERR, "dynamic module import is not implemented")
> +MSG_DEF(JSMSG_IMPORT_SCRIPT_NOT_FOUND,   0, JSEXN_TYPEERR, "can't referrencing script for dynamic module import")

Nit: "can't reference" or "can't find referencing"?

::: js/src/jsapi.h
@@ +3049,5 @@
>   */
>  extern JS_PUBLIC_API(void)
>  SetModuleMetadataHook(JSRuntime* rt, ModuleMetadataHook func);
>  
> +using ModuleDynamicImportHook = bool (*)(JSContext*, HandleValue, HandleString, HandleObject);

It might be nice to add the referencingPrivate/specifier/promise argument names as a form of documentation + some info about how the hook relates to FinishDynamicModuleImport.

::: js/src/vm/JSScript.cpp
@@ +5078,5 @@
>  
>  bool
>  JSScript::mayReadFrameArgsDirectly()
>  {
> +    return is<ModuleObject>() || argumentsHasVarBinding() || hasRest();

I don't understand why this change is necessary. r=me without it.
Attachment #9017551 - Flags: review?(jdemooij) → review+
Comment on attachment 9017557 [details] [diff] [review]
bug1342012-compile-dynamic-import

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

Some suggestions below that will hopefully help.

::: js/src/jit/CodeGenerator.cpp
@@ +3032,5 @@
> +
> +void
> +CodeGenerator::visitDynamicImport(LDynamicImport* lir)
> +{
> +    pushArg(ToRegister(lir->specifier()));

I think this should be something like:

pushArg(ToValue(lir, LDynamicImport::SpecifierIndex));
pushArg(ToValue(lir, LDynamicImport::ReferencingPrivateIndex));

Similar to: https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/js/src/jit/CodeGenerator.cpp#6763

Pushing a constant Value (stored in MConstant) could work but is a bit more complicated and probably not worth it.

::: js/src/jit/Lowering.cpp
@@ +2604,5 @@
>  
>  void
> +LIRGenerator::visitDynamicImport(MDynamicImport* ins)
> +{
> +    LDynamicImport* lir = new(alloc()) LDynamicImport(useRegisterAtStart(ins->specifier()));

I think this should be useBoxAtStart for both the specifier and private.

::: js/src/jit/MIR.h
@@ +7819,5 @@
>      }
>  };
>  
> +class MDynamicImport : public MBinaryInstruction,
> +                       public NoTypePolicy::Data

Here we can use "public BoxInputsPolicy::Data" to ensure we get two MIRType::Value operands.

@@ +7833,5 @@
> +    TRIVIAL_NEW_WRAPPERS
> +    NAMED_OPERANDS((0, referencingPrivate))
> +    NAMED_OPERANDS((1, specifier))
> +
> +    AliasSet getAliasSet() const override {

I think we should remove this. StartDynamicModuleImport calls ToString and we do Promise stuff so we probably have side-effects.

::: js/src/jit/shared/LIR-shared.h
@@ +4902,5 @@
>        : LCallInstructionHelper(classOpcode)
>      {}
>  };
>  
> +class LDynamicImport : public LCallInstructionHelper<1, BOX_PIECES, 0>

2 * BOX_PIECES (2 Values)

@@ +4907,5 @@
> +{
> +  public:
> +    LIR_HEADER(DynamicImport)
> +
> +    explicit LDynamicImport(const LAllocation& specifier)

2 LBoxAllocations for the specifier/private.
Attachment #9017557 - Flags: feedback?(jdemooij) → feedback+
Comment on attachment 9017562 [details] [diff] [review]
bug1342012-shell-import

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

\o/

::: js/src/jit-test/tests/modules/dynamic-import-script.js
@@ +1,1 @@
> +// |jit-test|

Nit: can remove

::: js/src/shell/js.cpp
@@ +851,5 @@
>      }
>  }
>  
> +static bool
> +RegisterScriptPathWithModuleLoader(JSContext* cx, HandleScript script, const char* filename)

This seems okay if it's shell only and it will probably be easier to refactor after this lands.

@@ +4929,5 @@
> +                                  "abortDynamicModuleImport", "0", "s");
> +        return false;
> +    }
> +
> +    if (!args[1].isString())

Nit: {} a few times.
Attachment #9017562 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #4)

> > +        if (!cx->runtime()->moduleDynamicImportHook) {
> > +            reportError(nullptr, JSMSG_NO_DYNAMIC_IMPORT);
> 
> Should we change this message to something saying the hook isn't installed?
> Or just MOZ_CRASH with a message saying the embedder has to install the hook?

My plan was to use the presence of the hook as the switch for enabling dynamic import.  With these patches applied the shell will support it but the browser will continue to throw with a message about it not being implemented.

> It might be nice to add the referencingPrivate/specifier/promise argument
> names as a form of documentation + some info about how the hook relates to
> FinishDynamicModuleImport.

Good idea.

> I don't understand why this change is necessary. r=me without it.

Ugh, sorry that must be a messed up merge.  I don't understand how that even compiles.
Thanks for the reviews.  How does this look?  The arm tests are green now.
Attachment #9017557 - Attachment is obsolete: true
Attachment #9017967 - Flags: review?(jdemooij)
Final patch.  I added an oomTest() and this showed up some assumptions that don't seem to hold under OOM.  The change means we only reset a module's status to 'uninstantiated' if we fail while it its status is 'instantiating', otherwise we leave it alone.
Attachment #9017974 - Flags: review?(jdemooij)
Comment on attachment 9017967 [details] [diff] [review]
bug1342012-compile-dynamic-import v2

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

::: js/src/jit/shared/LIR-shared.h
@@ +4908,5 @@
> +  public:
> +    LIR_HEADER(DynamicImport)
> +
> +    static const size_t ReferencingPrivateIndex = 1;
> +    static const size_t SpecifierIndex = 1;

The first should be |= 0|, the second should be |= BOX_PIECES|, like here:

https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/js/src/jit/shared/LIR-shared.h#2444-2445

Maybe add a masm.assumeUnreachable("dynamic import"); in the CodeGenerator code to double check jit-tests exercise that code, because I think it should have failed/crashed at runtime. Or we passed the wrong Value to the C++ function and that happened to work?
Attachment #9017967 - Flags: review?(jdemooij) → review+
Attachment #9017974 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #10)
Ugh, I'm sorry.

It turns out that the Ion compiled version of the test is not executed when it includes the call to promise.then() after import().  Running it twice in a loop fixes this.  I'll fix the problem and add version of the test that hits this.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/430db29f4685
Implement support for dynamic module import in the interpreter r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/6592655e860e
Compile dynamic module import bytecode r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/41812db6caba
Support dynamic module import in the shell r=jandem
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a836d54d21da
Add OOM tests for shell dynamic module import and fix bugs r=jandem
https://hg.mozilla.org/projects/larch/rev/430db29f46858faff930e2ec3ed45fbf13a73a20
Bug 1499140 - Implement support for dynamic module import in the interpreter r=jandem

https://hg.mozilla.org/projects/larch/rev/6592655e860e73040175e3a0de9d7a1ade2dea89
Bug 1499140 - Compile dynamic module import bytecode r=jandem

https://hg.mozilla.org/projects/larch/rev/41812db6cabaf796d6672786efa109ab771e3b2f
Bug 1499140 - Support dynamic module import in the shell r=jandem

https://hg.mozilla.org/projects/larch/rev/a836d54d21da033ae12512de20e68b21454df06b
Bug 1499140 - Add OOM tests for shell dynamic module import and fix bugs r=jandem
You need to log in before you can comment on or make changes to this bug.