Closed Bug 1219288 Opened 5 years ago Closed 5 years ago

Optimise access to import bindings in modules

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 3 obsolete files)

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.
Attachment #8680058 - Flags: review?(shu)
Attached patch imports-2-get-import-instruction (obsolete) — Splinter Review
Attachment #8680059 - Flags: review?(shu)
Attachment #8680060 - Flags: review?(shu)
Attached patch imports-4-optimise-ion (obsolete) — Splinter Review
Attachment #8680061 - Flags: review?(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?
Attachment #8680059 - Flags: review?(shu) → review+
Attachment #8680060 - Flags: review?(shu) → review+
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?
Attachment #8680061 - Flags: review?(shu)
Attachment #8680058 - Flags: review?(shu)
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.
Attachment #8686106 - Flags: review?(shu)
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.
Attachment #8680058 - Attachment is obsolete: true
Attachment #8686110 - Flags: review?(shu)
The only change in this version of the patch is that GetImportOperation() in Interpreter.cpp is now fallible because the import can be uninitialized.
Attachment #8680059 - Attachment is obsolete: true
Attachment #8686112 - Flags: review?(shu)
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.
Attachment #8680061 - Attachment is obsolete: true
Attachment #8686114 - Flags: review?(shu)
Attached patch time-importsSplinter Review
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 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.
Attachment #8686106 - Flags: review?(shu) → review+
Comment on attachment 8686110 [details] [diff] [review]
imports-1-store-shape-in-bindings v2

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

LGTM
Attachment #8686110 - Flags: review?(shu) → review+
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.
Attachment #8686112 - Flags: review?(shu) → review+
Attachment #8686114 - Flags: review?(shu) → review+
(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.
You need to log in before you can comment on or make changes to this bug.