1233109 - Store fewer bindings in module environments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

At the moment we alias all bindings at the top level of a module and store them in the module environment object.  This is unnecessary and we could get away with storing them on the stack if they are not exported or not otherwise aliased.

Comment 1

[Jon Coppeard (:jonco)]

This required a bit of refactoring first, so it's split into several patches.

The idea is this:
 - instantiate ModuleBuilder earlier and use it to gather data about imports and exports while we are parsing, rather than doing this at the end
 - use this data to mark exported local bindings as aliased before we generate the bindings
 - update the parser to mark other declarations aliased as necessary (e.g. module namespace imports)

We can also get rid of the list of exported names that's currently stored in the ModuleBox as we will have this information in the ModuleBuilder.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bindings-1-instantiate-builder-earlier

Instantiate ModuleBuilder before parsing and pass it in to standaloneModule().

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bindings-2-call-builder-during-parse

Rather than having ModuleBuilder traverse the AST after parsing, call into it every time an import/export parse node is created.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bindings-3-check-export-names-with-builder-data

Check for duplicate exported names using the data in the ModuleBuilder and remove ModuleBox::exportNames.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bindings-4-make-accessors-const

Make some accessors const.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bindings-5-alias-fewer-bindings

Finally we get to point of all this: don't automatically alias all declarations at module top level but use the builder's export information to alias exported top level bindings.

Currently we still need to alias all top level function definitions because of the way we instantiate function declarations ahead of executing the top level code.  It may be possible to do better by instantiating only some functions early.  I'll look into that when I get back next year.

This change means that Reflect.parse now throws syntax errors for module code that exports non-existent bindings, as it should.  Test code was updated for that and to reflect the different contents of the module environment.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8699108 [details] [diff] [review]
bindings-5-alias-fewer-bindings

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

This is simpler than I imagined, very nice.

Could you please write a Debugger test or two (crib off of the Environment-* tests inside jit-test/tests/debug) to make sure that these unaliased bindings are able to be reflected and fetched by the Debugger API?

::: js/src/frontend/Parser.cpp
@@ -305,5 @@
>              if (!checkLocalsOverflow(ts))
>                  return false;
>          }
> -        if (atModuleScope())
> -            dn->pn_dflags |= PND_CLOSED;

\o/

@@ +988,5 @@
> +        Definition* def = modulepc.decls().lookupFirst(name);
> +        if (!def) {
> +            JSAutoByteString str;
> +            if (!str.encodeLatin1(context, name))
> +                return null();

Why encodeLatin1 here instead of AtomToPrintableString?

::: js/src/jit-test/tests/modules/export-declaration.js
@@ +134,5 @@
>          ],
>          null,
>          false
>      )
> +]).assert(parseAsModule("let as = 1; export { as as as }"));

lol

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a605806d9f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7013426d292
https://hg.mozilla.org/integration/mozilla-inbound/rev/95ad4f2242ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/53e00d75c914
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb17cb935f2

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a110885c2b5b

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a605806d9f0
https://hg.mozilla.org/mozilla-central/rev/c7013426d292
https://hg.mozilla.org/mozilla-central/rev/95ad4f2242ef
https://hg.mozilla.org/mozilla-central/rev/53e00d75c914
https://hg.mozilla.org/mozilla-central/rev/3bb17cb935f2
https://hg.mozilla.org/mozilla-central/rev/a110885c2b5b