1169745 - Make SuperProperty work in the JITs

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P5)

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 0: Allow hop-counting through eval scopes in frontend.

We can only tell if we have a dynamic scope corresponding to a StaticEvalObject based on its marked strictness. Currently, we mark that strictness after emitting, which is too late to use in the frontend. Instead, seed it with the initial strictness of the script, and mark it immediately when parsing the directive.

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 1: Rework JSOP_SUPERBASE to take hops

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8708576 [details] [diff] [review]
Part 0: Allow hop-counting through eval scopes in frontend.

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

It's your lucky day - I am not suspicious and/or knowledgeable enough about what could go wrong here to bury you in requests for new tests.

I kind of doubt we have solid test coverage of this stuff, though, so please do at least add:

* a simple test where it actually goes off and we emit a GET/SETALIASEDVAR;
* and one with GET/SETGNAME, assuming that is actually possible;
* and one where we *can't* optimize some use of an outer variable, because of some enclosing non-strict scope.

Make sure the tests hit the eval cache, too, to make sure that doesn't break anything (though I don't see how it could).

::: js/src/builtin/Eval.cpp
@@ +395,5 @@
>  
> +        // Seed the strictness. Any directives will set the scope's strictness
> +        // in CompileScript.
> +        if (options.strictOption)
> +            staticScope->setStrict();

No --- we've had two near-identical copies of this code for long enough. Common up at least the parts that appear in your -U8 patch, and as much more as is convenient. Even if there are 23 parameters, it'll still save lines of code.

I mean, what the hell.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8708578 [details] [diff] [review]
Part 1: Rework JSOP_SUPERBASE to take hops

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

r=me, fun patch!

::: js/src/vm/Interpreter.cpp
@@ +3896,5 @@
> +    MOZ_ASSERT(callee.allowSuperProperty());
> +    MOZ_ASSERT(callee.nonLazyScript()->needsHomeObject());
> +
> +    const Value& homeObjVal = callee.getExtendedSlot(FunctionExtended::METHOD_HOMEOBJECT_SLOT);
> +    RootedNativeObject &homeObj = rootNativeObject0;

ReservedRooted<NativeObject*>, right? (but if not, style nit: move the & left a space - it's supposed to cuddle the type now).

@@ +3899,5 @@
> +    const Value& homeObjVal = callee.getExtendedSlot(FunctionExtended::METHOD_HOMEOBJECT_SLOT);
> +    RootedNativeObject &homeObj = rootNativeObject0;
> +    homeObj = &homeObjVal.toObject().as<NativeObject>();
> +
> +    RootedObject& superBase = rootObject1;

and here

Comment 5

[Ted Campbell [:tcampbell]]

Comment on attachment 8708576 [details] [diff] [review]
Part 0: Allow hop-counting through eval scopes in frontend.

The frontend rewrite removes the need for this patch.

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: Bug 1169745 - Support JSOP_SUPERBASE in Baseline

Review commit: https://reviewboard.mozilla.org/r/145478/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145478/

Comment 7

[Ted Campbell [:tcampbell]]

Functional patch is up. I'll do a little more rework before review and support for JSOP_SUPERFUN.

As it stands, I'm not sure changing the bytecode helps much so am opting not to do it. I use BaselineFrame::calleeToken to avoid the environment walk in normal (non-eval, non-arrow) cases.

Comment 8

[Ted Campbell [:tcampbell]]

Moving JSOP_SUPERFUN changes to Bug 1169746.

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1169745-JSOP_GETPROP_SUPER-should-Typescript-Mon.patch

JSOP_GETPROP_SUPER was missing TI update in interpreter.

Comment 10

[Ted Campbell [:tcampbell]]

Attachment: 0002-Bug-1169745-Support-JSOP_SUPERBASE-in-Baseline.patch

Support the JSOP_SUPERBASE opcode in baseline to allow |super.x|.

One optimization is in typical case I use frame.callee to get HomeObject rather than walking the environment.

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 8875481 [details] [diff] [review]
0002-Bug-1169745-Support-JSOP_SUPERBASE-in-Baseline.patch

Allow |super.x| in Baseline.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8875481 [details] [diff] [review]
0002-Bug-1169745-Support-JSOP_SUPERBASE-in-Baseline.patch

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

Very nice to have the inline path.

::: js/src/jit/BaselineCompiler.cpp
@@ +4163,5 @@
> +        if (si.hasSyntacticEnvironment() && si.scope()->is<FunctionScope>()) {
> +            JSFunction* fn = si.scope()->as<FunctionScope>().canonicalFunction();
> +
> +            if (!fn->isArrow())
> +                break;;

Nit: s/;;/;/

@@ +4168,5 @@
> +        }
> +
> +        // Traverse environment chain
> +        if (si.scope()->hasEnvironment()) {
> +            Address next_addr(reg, EnvironmentObject::offsetOfEnclosingEnvironment());

Nit: nextAddr

@@ +4193,5 @@
> +    // Lookup callee object of environment containing [[ThisValue]]
> +    getThisEnvironmentCallee(scratch);
> +
> +    // Load [[HomeObject]]
> +    Address homeobj_addr(scratch, FunctionExtended::offsetOfMethodHomeObjectSlot());

Nit: homeObjAddr

::: js/src/jit/VMFunctions.cpp
@@ +1804,5 @@
>      return true;
>  }
>  
> +JSObject*
> +HomeObjectSuperBase(JSContext* cx, HandleObject homeObj)

Please move this function to vm/Interpreter.cpp/h and call it in the interpreter for JSOP_SUPERBASE, to avoid duplicating the logic.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c63e64430f
JSOP_GETPROP_SUPER should Typescript::Monitor. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/532539a21d57
Support JSOP_SUPERBASE in Baseline. r=jandem

Comment 14

[Ted Campbell [:tcampbell]]

TODO:

- Attach TypeMonitor_AnyValue IC for supercall TDZ values so they don't VMCall on every run.
- Refactor to add IsConstructorCallPC helper to reduce special cases of JSOP_SUPERCALL.
- Refactor branchIfFunction* helpers to use common helper and add branchIfNotConstructor.

Comment 15

[Ted Campbell [:tcampbell]]

Oops, wrong bug. TODO list for here is to support JSOP_GETPROP_SUPER and friends.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f8c63e64430f
https://hg.mozilla.org/mozilla-central/rev/532539a21d57

Comment 17

[Tom S [:evilpie]]

See comment 15, we don't actually support this yet.

Comment 18

[Ted Campbell [:tcampbell]]

Attachment: 0003-Bug-1169745-Fix-vm-Opcodes.h-for-JSOP_GETPROP_SUPER-.patch

Turns out the stack-order documentation in vm/Opcodes.h was inconsistent with the interpreter code.

As well, the JOF_TYPESET annotations were missing. These will force typeset locations to be allocated that may not have before, but I haven't had any issues in various try runs this week.

Comment 19

[Ted Campbell [:tcampbell]]

Attachment: 0004-Bug-1169745-Support-JSOP_GETPROP_SUPER-in-Baseline.-.patch

JSOP_GETPROP_SUPER Baseline is ready for review.

I used GetPropIRGenerator to take advantage of all the good work there. I checked the existing stub variants as best I could and included tests where they made sense. I explicitly avoid Proxies for now since |this| behavior is tricky. Native, Unboxed, megamorphic cases should all work.

One area that is a little weak is the SharedIC.cpp changes requiring duplicating DoGetPropFallback due to different arguments and tail-call trampoline needed. Ideas welcome to clean that up (possibly a follow-up patch).

Comment 20

[Ted Campbell [:tcampbell]]

Try run for GETPROP_SUPER: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7193960b2dc147d46925682202ec9dc88ec8909&group_state=expanded

Comment 21

[Ted Campbell [:tcampbell]]

Attachment: 0005-Bug-1169745-Support-JSOP_GETELEM_SUPER-in-Baseline.-.patch

Follow up to patch 0004 with GETELEM_SUPER support.

Comment 22

[Ted Campbell [:tcampbell]]

Try run for GETELEM_SUPER (and GETPROP_SUPER included): https://treeherder.mozilla.org/#/jobs?repo=try&revision=663279749a8827989af672c1a49813b9892351ad&group_state=expanded

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 8880564 [details] [diff] [review]
0003-Bug-1169745-Fix-vm-Opcodes.h-for-JSOP_GETPROP_SUPER-.patch

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

Good find.

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 8880843 [details] [diff] [review]
0004-Bug-1169745-Support-JSOP_GETPROP_SUPER-in-Baseline.-.patch

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

This is super nice (no pun intended). Very cool we can support even getters with only minor CacheIR emitter changes.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1951,5 @@
>  #endif
>          break;
> +#if DEBUG
> +      default:
> +        MOZ_CRASH("Unsupported IC");

We usually don't add the default case when it's not needed, to ensure the compiler warns when we add a new enum value. If you want to add it I'd remove the #if.

@@ +2010,5 @@
>          stubKind = CacheIRStubKind::Updated;
>          break;
> +#if DEBUG
> +      default:
> +        MOZ_CRASH("Unsupported IC");

Same here.

::: js/src/jit/CacheIR.cpp
@@ +604,5 @@
>          trackAttached("NativeSlot");
>          return true;
> +      case CanAttachCallGetter: {
> +        // |super.prop| accesses use a |this| value that differs from lookup object
> +        ObjOperandId receiverId = isSuper() ? writer.guardIsObject(getThisValueId())

See bug 1337812. Similar to that, I wonder if it would be nicer to pass the receiver as Maybe<ValOperandId> or something, to make things more explicit. r=me either way.

@@ +994,5 @@
>      if (mode_ == ICState::Mode::Megamorphic)
>          return tryAttachGenericProxy(obj, objId, id, /* handleDOMProxies = */ true);
>  
> +    // The proxy stubs don't currently support |super| access.
> +    if (isSuper())

Shouldn't we move this check before the tryAttachGenericProxy call? We can get here when a class inherits from a proxy?

::: js/src/jit/CacheIR.h
@@ +1136,5 @@
>          MOZ_ASSERT(cacheKind_ == CacheKind::GetElem);
>          return ValOperandId(1);
>      }
>  
> +    ValOperandId getThisValueId() const {

Maybe getReceiverValueId? Or even getSuperReceiverValueId, it's pretty easy to confuse them.

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 8880902 [details] [diff] [review]
0005-Bug-1169745-Support-JSOP_GETELEM_SUPER-in-Baseline.-.patch

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

::: js/src/jit/BaselineIC.h
@@ +415,5 @@
>  
>      // Compiler for this stub kind.
>      class Compiler : public ICStubCompiler {
>        protected:
> +        bool hasReceiver_;

IIRC the previous patch called this hasThis_. We should probably change it to use hasReceiver_, too.

Comment 26

[Ted Campbell [:tcampbell]]

Good catch on the isSuper after megamorphic check. I must have scrambled it up when cleaning up patches.

I'll also s/this/receiver/.

I'll remove the default cases. I had put them in because I forgot one of those cases and didn't notice the warnings.

I'll leave the getXXXValueId vs Maybe<> stuff for now and add a comment to that bug.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27e20733c500
Fix vm/Opcodes.h for JSOP_GETPROP_SUPER / JSOP_GETELEM_SUPER. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/da8bb4db80e5
Support JSOP_GETPROP_SUPER in Baseline. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/167c7d1eaee1
Support JSOP_GETELEM_SUPER in Baseline. r=jandem

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27e20733c500
https://hg.mozilla.org/mozilla-central/rev/da8bb4db80e5
https://hg.mozilla.org/mozilla-central/rev/167c7d1eaee1

Comment 29

[Mayank Bansal]

This probably improved the score on Six-speed by 27% https://arewefastyet.com/#machine=29&view=breakdown&suite=six-speed   by improving the score of https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=super-es6 by 72%

Comment 30

[Kris Maglione [:kmag]]

Looks like this improved add-on manager and system add-on startup by about 8-9% (~10ms on my machine), and some areas of the WebExtension framework by >15%.

This also shows up in talos as about a 10ms improvement on win64 ts_paint opt.