Following on from bug 1219288, we need to optimise access to namespaces imports in modules (i.e. |import * as name from 'foo'|). This is not quite so straightforward because as well as accesses we know about statically, the namespace is a regular JS object and can be copied, passed around, accessed via subscript syntax, etc.

Add a baseline GetProp stub for module namespace objects.

Attempt to optimise getprop on a module namespace in Ion based on type information.

Add an Ion IC to handle cases where don't have a singleton type.

Comment on attachment 8692603 [details] [diff] [review] optimise-ns-baseline Review of attachment 8692603 [details] [diff] [review]: ----------------------------------------------------------------- Refresh my memory (this must be the case): ModuleNamespaceObjects are sealed for all intents and purposes, right? No property's gonna be deleted or reconfigure or anything like that? ::: js/src/jit/SharedIC.cpp @@ +4345,5 @@ > + masm.loadPtr(Address(loadBase, NativeObject::offsetOfSlots()), loadBase); > + > + // Load the property. > + masm.load32(Address(ICStubReg, ICGetProp_ModuleNamespace::offsetOfOffset()), scratch); > + masm.loadValue(BaseIndex(loadBase, scratch, TimesOne), R0); Why BaseIndex here instead of Address, if the scale is 1? ::: js/src/jit/SharedIC.h @@ +2818,5 @@ > + uint32_t offset) > + : ICMonitoredStub(ICStub::GetProp_ModuleNamespace, stubCode, firstMonitorStub), > + namespace_(ns), environment_(env), offset_(offset) > + { > + (void) offset_; // Silence clang warning. wat

Comment on attachment 8692604 [details] [diff] [review] optimise-ns-ion-ti Review of attachment 8692604 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding the tracked optimization info. :) I suggested some refactorings below. No need to re-request r? for them, but I'll be happy to look at them if you want to. ::: js/src/jit/IonBuilder.cpp @@ +11385,5 @@ > + ModuleNamespaceObject* ns = &singleton->as<ModuleNamespaceObject>(); > + ModuleEnvironmentObject* env; > + Shape* shape; > + if (!ns->bindings().lookup(NameToId(name), &env, &shape)) { > + trackOptimizationOutcome(TrackedOutcome::UnknownProperty ); Nit: extra space before ) @@ +11398,5 @@ > + > + MConstant* nsConst = constant(ObjectValue(*ns)); > + MGuardObjectIdentity* guard = MGuardObjectIdentity::New(alloc(), obj, nsConst, > + /* bailOnEquality = */ false); > + current->add(guard); Just to make sure I understand, the MGuardObject is to get the type policies to work out and to ensure an unbox happens so it can go into the identity guard, right? @@ +11419,5 @@ > + current->add(load); > + current->push(load); > + > + if (!pushTypeBarrier(load, types, barrier)) > + return false; For the code from the |MConstant* envConst| line until here, is it possible to use IonBuilder::loadSlot?

Comment on attachment 8692604 [details] [diff] [review] optimise-ns-ion-ti Review of attachment 8692604 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonBuilder.cpp @@ +11398,5 @@ > + > + MConstant* nsConst = constant(ObjectValue(*ns)); > + MGuardObjectIdentity* guard = MGuardObjectIdentity::New(alloc(), obj, nsConst, > + /* bailOnEquality = */ false); > + current->add(guard); Actually, thinking about this again, since you already require that obj is a singleton, I imagine TI already has the constraints in place that does *not* require you to do another GuardObjectIdentity.

Comment on attachment 8692606 [details] [diff] [review] optimise-ns-ion-cache Review of attachment 8692606 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonCaches.cpp @@ +2059,5 @@ > + > + // If we need a scratch register, use either an output register or the > + // object register. > + bool restoreScratch = false; > + Register scratchReg = Register::FromCode(0); // Quell compiler warning. Use InvalidReg. @@ +2065,5 @@ > + if (output.hasValue()) { > + scratchReg = output.valueReg().scratchReg(); > + } else if (output.type() == MIRType_Double) { > + scratchReg = object; > + masm.push(scratchReg); Totally subjective nit: I feel like this would be clearer if it were masm.push(object); scratchReg = object; @@ +2078,5 @@ > + EmitLoadSlot(masm, &env->as<NativeObject>(), shape, envReg, output, scratchReg); > + > + // Restore scratch on success. > + if (restoreScratch) > + masm.pop(scratchReg); Paired totally subjective nit, if you choose to apply the comment above: masm.pop(object); @@ +2122,5 @@ > + Label* maybeFailures = failures.used() ? &failures : nullptr; > + > + GenerateReadModuleNamespace(cx, ion, masm, attacher, ns, env, > + shape, object(), output(), maybeFailures); > + attachKind = idempotent() ? "idempotent reading" : "non idempotent reading"; I get the feeling that all namespace getprops are going to idempotent? Could you change this to an assert and see if it ever fails? In any case, please change the string to something like "module namespace".

(In reply to Shu-yu Guo [:shu] from comment #4) > Refresh my memory (this must be the case): ModuleNamespaceObjects are sealed > for all intents and purposes, right? No property's gonna be deleted or > reconfigure or anything like that? Yes. We're accessing the target ModuleEnvironmentObject here, and these are created non-extensible with all properties non-configurable. > > + masm.loadValue(BaseIndex(loadBase, scratch, TimesOne), R0); > > Why BaseIndex here instead of Address, if the scale is 1? Address takes a constant offset and here scratch is a register containing the offset we loaded from the IC data. > > + (void) offset_; // Silence clang warning. > > wat Clang complains because it thinks the field is never used.

(In reply to Shu-yu Guo [:shu] from comment #6) > Actually, thinking about this again, since you already require that obj is a > singleton, I imagine TI already has the constraints in place that does *not* > require you to do another GuardObjectIdentity. I added some tests and found that this is correct, neither the GuardObject or GuardObjectIdentity are required. I don't really understand how this happens though. The tests did show up a couple of issues with the baseline patch, however: - I used branchTestPtr() where I should have used branchPtr() to guard on object identity - I missed adding the new IC to the switch statement in BaselineInspector::expectedPropertyAccessInputType() I'll post an updated patch in case you want to look at it again.

Updated patch.

Add a couple of tests to check we have the correct guards in place when optimising module namespace imports.

(In reply to Shu-yu Guo [:shu] from comment #7) > I get the feeling that all namespace getprops are going to idempotent? Could > you change this to an assert and see if it ever fails? I added an assert, and yes it sometimes fails. Other comments applied.

https://hg.mozilla.org/integration/mozilla-inbound/rev/67dc020babd8 https://hg.mozilla.org/integration/mozilla-inbound/rev/45defe7e1a19 https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a08ea6aace

https://hg.mozilla.org/mozilla-central/rev/67dc020babd8 https://hg.mozilla.org/mozilla-central/rev/45defe7e1a19 https://hg.mozilla.org/mozilla-central/rev/e2a08ea6aace