Make SuperProperty work in the JITs

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript Engine: JIT
P5
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: efaust, Assigned: tcampbell)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

2 years ago
Created attachment 8708576 [details] [diff] [review]
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.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8708576 - Flags: review?(jorendorff)
(Reporter)

Comment 2

2 years ago
Created attachment 8708578 [details] [diff] [review]
Part 1: Rework JSOP_SUPERBASE to take hops
Attachment #8708578 - Flags: review?(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.
Attachment #8708576 - Flags: review?(jorendorff) → review+
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
Attachment #8708578 - Flags: review?(jorendorff) → review+

Updated

11 months ago
Assignee: efaustbmo → nobody
Status: ASSIGNED → NEW
Priority: -- → P5

Updated

11 months ago
Assignee: nobody → efaustbmo
(Assignee)

Updated

3 months ago
Assignee: efaustbmo → tcampbell
(Assignee)

Comment 5

3 months ago
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.
Attachment #8708576 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 7

3 months ago
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.
(Assignee)

Comment 8

3 months ago
Moving JSOP_SUPERFUN changes to Bug 1169746.
(Assignee)

Updated

3 months ago
Attachment #8708578 - Attachment is obsolete: true
(Assignee)

Comment 9

3 months ago
Created attachment 8875480 [details] [diff] [review]
0001-Bug-1169745-JSOP_GETPROP_SUPER-should-Typescript-Mon.patch

JSOP_GETPROP_SUPER was missing TI update in interpreter.
Attachment #8875480 - Flags: review?(jdemooij)
(Assignee)

Comment 10

3 months ago
Created attachment 8875481 [details] [diff] [review]
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.
Attachment #8874039 - Attachment is obsolete: true
(Assignee)

Comment 11

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

Allow |super.x| in Baseline.
Attachment #8875481 - Flags: review?(jdemooij)

Updated

2 months ago
Attachment #8875480 - Flags: review?(jdemooij) → review+
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.
Attachment #8875481 - Flags: review?(jdemooij) → review+

Comment 13

2 months ago
Pushed by tcampbell@mozilla.com:
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
(Assignee)

Comment 14

2 months ago
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.
(Assignee)

Comment 15

2 months ago
Oops, wrong bug. TODO list for here is to support JSOP_GETPROP_SUPER and friends.
https://hg.mozilla.org/mozilla-central/rev/f8c63e64430f
https://hg.mozilla.org/mozilla-central/rev/532539a21d57
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
See comment 15, we don't actually support this yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 months ago
Depends on: 1374462
(Assignee)

Comment 18

2 months ago
Created attachment 8880564 [details] [diff] [review]
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.
Attachment #8880564 - Flags: review?(jdemooij)
(Assignee)

Comment 19

2 months ago
Created attachment 8880843 [details] [diff] [review]
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).
Attachment #8880843 - Flags: review?(jdemooij)
(Assignee)

Comment 20

2 months ago
Try run for GETPROP_SUPER: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7193960b2dc147d46925682202ec9dc88ec8909&group_state=expanded
(Assignee)

Comment 21

2 months ago
Created attachment 8880902 [details] [diff] [review]
0005-Bug-1169745-Support-JSOP_GETELEM_SUPER-in-Baseline.-.patch

Follow up to patch 0004 with GETELEM_SUPER support.
Attachment #8880902 - Flags: review?(jdemooij)
(Assignee)

Comment 22

2 months ago
Try run for GETELEM_SUPER (and GETPROP_SUPER included): https://treeherder.mozilla.org/#/jobs?repo=try&revision=663279749a8827989af672c1a49813b9892351ad&group_state=expanded
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.
Attachment #8880564 - Flags: review?(jdemooij) → review+
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.
Attachment #8880843 - Flags: review?(jdemooij) → review+
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.
Attachment #8880902 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 26

2 months ago
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

2 months ago
Pushed by tcampbell@mozilla.com:
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/27e20733c500
https://hg.mozilla.org/mozilla-central/rev/da8bb4db80e5
https://hg.mozilla.org/mozilla-central/rev/167c7d1eaee1
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
Resolution: --- → FIXED

Comment 29

2 months ago
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%
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.
You need to log in before you can comment on or make changes to this bug.