ModuleObject entrains the top level script

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P5
normal
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks 1 bug)

61 Branch
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

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

Comment 1

7 months ago
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)
Assignee

Comment 2

7 months ago
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]
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.)
Attachment #9015590 - Flags: review?(sphink) → review+
Assignee

Comment 4

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

Moving review since Jan is on PTO.
Attachment #9015588 - Flags: review?(jdemooij) → review?(nicolas.b.pierron)
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
Attachment #9015588 - Flags: review?(nicolas.b.pierron) → review+
Assignee

Comment 6

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

Comment 7

7 months ago
Pushed by jcoppeard@mozilla.com:
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

7 months ago
Pushed by archaeopteryx@coole-files.de:
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f73e13de8e71
https://hg.mozilla.org/mozilla-central/rev/f8b19c4105d2
https://hg.mozilla.org/mozilla-central/rev/bf766a8de9d7
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1498980
You need to log in before you can comment on or make changes to this bug.