1489477 - ModuleObject entrains the top level script

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P5)

Description

[Jon Coppeard (:jonco)]

The top level JSScript for a module is only needed the first time the module is evaluated, but is kept alive by the ModuleObject.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1489477-jit-properly

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.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1489477-dont-entrain-script

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.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9015590 [details] [diff] [review]
bug1489477-dont-entrain-script

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.)

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 9015588 [details] [diff] [review]
bug1489477-jit-properly

Moving review since Jan is on PTO.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9015588 [details] [diff] [review]
bug1489477-jit-properly

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.


[1] https://searchfox.org/mozilla-central/source/js/src/jit/MIR.h#1182-1186
[2] https://searchfox.org/mozilla-central/source/js/src/jit/MIR.h#2294-2296

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
Ah thanks, I'll fix that.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f73e13de8e71
Don't pre-create module metadata object when compiling r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b19c4105d2
Stop modules from entraining the top-level JSScript r=sfink

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf766a8de9d7
Don't pre-create module metadata object when compiling: remove trailing space a=eslint-fix

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f73e13de8e71
https://hg.mozilla.org/mozilla-central/rev/f8b19c4105d2
https://hg.mozilla.org/mozilla-central/rev/bf766a8de9d7