Closed Bug 1489477 Opened 3 years ago Closed 3 years ago

ModuleObject entrains the top level script


(Core :: JavaScript Engine, enhancement, P5)

61 Branch



Tracking Status
firefox64 --- fixed


(Reporter: jonco, Assigned: jonco)


(Blocks 1 open bug)



(2 files)

The top level JSScript for a module is only needed the first time the module is evaluated, but is kept alive by the ModuleObject.
Annoyingly the shell import.meta hook currently uses the module's script to get its filename to use for the 'url' property, and to fix that I'm going to move the hook's implementation to self-hosted JS.  But that doesn't work at the moment because we can currently call this hook while compiling to pre-create the metadata object if it doesn't already exist.

This patch makes us generate code to create/get the metadata object at runtime just like happens in the interpreter.
Assignee: nobody → jcoppeard
Attachment #9015588 - Flags: review?(jdemooij)
Patch to split ModuleObject::script() in to script() and maybeScript() and move implementation of the shell import.meta hook to self-hosted JS so we can get the module path easily.  Mostly the latter.

The ModuleObjet's script is set to null after it has been executed so we don't hold onto it.

A bug in the module loader could probably cause us to try and get the script for a module who's script is null so I made the check a release assert.
Attachment #9015590 - Flags: review?(sphink)
Comment on attachment 9015590 [details] [diff] [review]

Review of attachment 9015590 [details] [diff] [review]:

::: js/src/builtin/ModuleObject.cpp
@@ +1163,5 @@
>      RootedScript script(cx, self->script());
> +
> +    // The top-level script if a module is only ever executed once. Clear the
> +    // reference to prevent us keeping this alive unnecessarily.
> +    self->setReservedSlot(ScriptSlot, UndefinedValue());

I doubt it matters enough to risk messing with, but it seems like we'll know that this is unreachable after this, so the pre-barrier might end up keeping it alive for one more cycle than is strictly necessary.

::: js/src/shell/js.cpp
@@ +8008,5 @@
> +    JS_FN_HELP("setModuleMetadataHook", SetModuleMetadataHook, 1, 0,
> +"setModuleMetadataHook(function(module) {})",
> +"  Set the HostPopulateImportMeta hook to |function|.\n"
> +"  This hook is used to create the metadata object returned by import.meta for\n"
> +"  a module.  It should be implemented by the module loader."),

I'll just trust you that this is the right extension point for testing. (I mean, it makes sense, but I don't know the spec.)
Attachment #9015590 - Flags: review?(sphink) → review+
Comment on attachment 9015588 [details] [diff] [review]

Moving review since Jan is on PTO.
Attachment #9015588 - Flags: review?(jdemooij) → review?(nicolas.b.pierron)
Comment on attachment 9015588 [details] [diff] [review]

Review of attachment 9015588 [details] [diff] [review]:

This patch looks good to me except that we need to record the CompilerObject of MModuleMetadata.
r=me with one of the 2 alternatives:

::: js/src/jit/MIR.h
@@ +7792,5 @@
>  };
> +class MModuleMetadata : public MNullaryInstruction
> +{
> +    CompilerObject module_;

You need to either override `appendRoots` [1], or use a Constant to hold this object value like done with many `templateObject` [2] which are made to be resumed with recover instructions.

Attachment #9015588 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #5)
Ah thanks, I'll fix that.
Pushed by
Don't pre-create module metadata object when compiling r=nbp
Stop modules from entraining the top-level JSScript r=sfink
Pushed by
Don't pre-create module metadata object when compiling: remove trailing space a=eslint-fix
You need to log in before you can comment on or make changes to this bug.