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 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 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?

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.

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.

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

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.

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.

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

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.

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

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

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