1219288 - Optimise access to import bindings in modules

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

This is bug is to optimise reads of import bindings, e.g. |import { x } from "module.js"|.  It does not address namespaces imports, i.e. |import * as ns from "module.js"| which are implemented separately.

The idea is to do the following:

1. Replace the name stored in an IndirectBinding with the corresponding shape in the target environment, because most of the time we will need to look this up anyway.

2. Create a new bytecode instruction to access these non-namespace imports.

3. Optimise the new instruction in baseline and ion.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: imports-1-store-shape-in-bindings

Comment 2

[Jon Coppeard (:jonco)]

Attachment: imports-2-get-import-instruction

Comment 3

[Jon Coppeard (:jonco)]

Attachment: imports-3-optimise-baseline

Comment 4

[Jon Coppeard (:jonco)]

Attachment: imports-4-optimise-ion

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8680058 [details] [diff] [review]
imports-1-store-shape-in-bindings

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

This looks okay, but storing shape pointers outside of the object they belong to without guards worry me in general.

I suppose in the case of modules this is okay because the target ModuleEnvironment is frozen at the point of instantiating the indirect imports. This approach wouldn't fly if the target ModuleEnvironment can have exports added or removed, or if the environment can go into dictionary mode. Can the environment go into dictionary mode? I hope not.

Could you try to figure out the list of invariants that makes it okay to store this shape pointer?

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8680061 [details] [diff] [review]
imports-4-optimise-ion

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

Looks good. Would like to see a new patch with a better fallback.

::: js/src/jit/IonBuilder.cpp
@@ +8311,5 @@
> +{
> +    ModuleEnvironmentObject* env = GetModuleEnvironmentForScript(script());
> +    MOZ_ASSERT(env);
> +
> +    jsid id = NameToId(script()->getName(pc));

Use the |name| that's already passed in.

@@ +8318,5 @@
> +    MOZ_ALWAYS_TRUE(env->lookupImport(id, &targetEnv, &shape));
> +
> +    PropertyName* localName = JSID_TO_STRING(shape->propid())->asAtom().asPropertyName();
> +    bool emitted = false;
> +    if (!getStaticName(targetEnv, localName, &emitted) || emitted)

Just checking: imported bindings can never be uninitialized (thus triggering TDZ) in the target module at this point, right?

@@ +8321,5 @@
> +    bool emitted = false;
> +    if (!getStaticName(targetEnv, localName, &emitted) || emitted)
> +        return emitted;
> +
> +    return jsop_getname(name);

We can do better than jsop_getname as a fallback. Since the shape is always slotful, this could be calling loadSlot here, no?

Comment 7

[Jon Coppeard (:jonco)]

Attachment: imports-0-protect-module-env

Holding on to shape pointers for the module environment is safe if:
 - we do not add or remove properties
 - we do not reconfigure properties
 - the object does not go into dictionary mode

To that end, this patch ensure that the module environment object is not extensible, and that its properties are not configurable.

To do this I had to change the way namespace import properties work, as these used to be created in the module declaration instantiation step.  With this patch they are created up front and their values set directly (by slot access to bypass configurability checks) when the module namespace binding is created.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: imports-1-store-shape-in-bindings v2

This is an updated version of the patch to store the shapes for indirect bindings that has better encapsulation of the bindings map and asserts that the shape is valid any time it is handed out.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: imports-2-get-import-instruction v2

The only change in this version of the patch is that GetImportOperation() in Interpreter.cpp is now fallible because the import can be uninitialized.

Comment 10

[Jon Coppeard (:jonco)]

Attachment: imports-4-optimise-ion

Looking at getStaticName(), I don't think this can actually fail for a module import, so I've remove the fallback and asserted success.  This passes the tests.

I added a uninitialized lexical check in the case where the import isn't initialized by the time we compile.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: time-imports

Here's a script I used to benchmark this, with the following results:

Interpreter only:
  local let:        111.949 nS
  local var:        99.585 nS
  local const:      131.044 nS
  local func:       201.501 nS
  imported let:     126.245 nS
  imported var:     124.632 nS
  imported const:   126.114 nS
  imported default: 124.844 nS
  imported func:    237.56 nS
Baseline only, no Ion:
  local let:        8.512 nS
  local var:        7.998 nS
  local const:      9.003 nS
  local func:       21.602 nS
  imported let:     9.002 nS
  imported var:     9.002 nS
  imported const:   8.488 nS
  imported default: 8.764 nS
  imported func:    21.639 nS
Ion and Baseline enabled:
  local let:        0.979 nS
  local var:        0.971 nS
  local const:      0.969 nS
  local func:       1.002 nS
  imported let:     0.958 nS
  imported var:     0.959 nS
  imported const:   0.962 nS
  imported default: 0.811 nS
  imported func:    0.988 nS

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8686106 [details] [diff] [review]
imports-0-protect-module-env

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

::: js/src/vm/SelfHosting.cpp
@@ +1388,2 @@
>      RootedId name(cx, AtomToId(&args[1].toString()->asAtom()));
> +    RootedValue ns(cx, args[2]);

No need to re-root. args[n] is already a MutableHandleValue.

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8686110 [details] [diff] [review]
imports-1-store-shape-in-bindings v2

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

LGTM

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8686112 [details] [diff] [review]
imports-2-get-import-instruction v2

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

::: js/src/frontend/Parser.cpp
@@ +4485,5 @@
>  template<>
>  ParseNode*
> +Parser<FullParseHandler>::newBoundImportForCurrentName(JSOp op)
> +{
> +    MOZ_ASSERT(op == JSOP_GETNAME || op == JSOP_GETIMPORT);

When does this get called with op == JSOP_GETIMPORT? I see that op's default value is JSOP_GETIMPORT, but its one callsite uses JSOP_GETNAME. Could you tighten this up?

::: js/src/vm/Interpreter.cpp
@@ +2849,5 @@
> +CASE(JSOP_GETIMPORT)
> +{
> +    ReservedRooted<Value> rval(&rootValue0);
> +    if (!GetImportOperation(cx, REGS.fp(), REGS.pc, &rval))
> +        goto error;

I think PUSH_NULL and then using stackHandleAt(-1) is a little better.

Comment 15

[Jon Coppeard (:jonco)]

(In reply to Shu-yu Guo [:shu] from comment #14)

> When does this get called with op == JSOP_GETIMPORT? I see that op's default
> value is JSOP_GETIMPORT, but its one callsite uses JSOP_GETNAME. Could you
> tighten this up?

It does get called in two places, but looking at this again it's confused and unnecessary.  The original purpose was to treat namespace imports differently from imported names, but that already happens with patch 0 in this set, so this parameter can be removed and we don't have to set the op on the new node at all.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42dfbaceb13c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff361d63802
https://hg.mozilla.org/integration/mozilla-inbound/rev/037f7cfaa141
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f71a6784a03
https://hg.mozilla.org/integration/mozilla-inbound/rev/1743f61658f1

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42dfbaceb13c
https://hg.mozilla.org/mozilla-central/rev/7ff361d63802
https://hg.mozilla.org/mozilla-central/rev/037f7cfaa141
https://hg.mozilla.org/mozilla-central/rev/3f71a6784a03
https://hg.mozilla.org/mozilla-central/rev/1743f61658f1