1227567 - Optimise access to module namespace imports

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

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.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: optimise-ns-baseline

Add a baseline GetProp stub for module namespace objects.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: optimise-ns-ion-ti

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

Comment 3

[Jon Coppeard (:jonco)]

Attachment: optimise-ns-ion-cache

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

Comment 4

[Shu-yu Guo [:shu]]

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 5

[Shu-yu Guo [:shu]]

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 6

[Shu-yu Guo [:shu]]

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 7

[Shu-yu Guo [:shu]]

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

Comment 8

[Jon Coppeard (:jonco)]

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

Comment 9

[Jon Coppeard (:jonco)]

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

Comment 10

[Jon Coppeard (:jonco)]

Attachment: optimise-ns-baseline v2

Updated patch.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: test-namepace-compilation

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

Comment 12

[Jon Coppeard (:jonco)]

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

Comment 13

[Pulsebot]

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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/801a1141bc38

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/801a1141bc38