Make SuperProperty work in the JITs

REOPENED
Assigned to

Status

()

Core
JavaScript Engine: JIT
P5
normal
REOPENED
2 years ago
23 hours ago

People

(Reporter: efaust, Assigned: tcampbell)

Tracking

(Depends on: 1 bug, 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

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

Updated

9 months ago
Assignee: nobody → efaustbmo
(Assignee)

Updated

29 days ago
Assignee: efaustbmo → tcampbell
(Assignee)

Comment 5

22 days 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

22 days 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

18 days ago
Moving JSOP_SUPERFUN changes to Bug 1169746.
(Assignee)

Updated

17 days ago
Attachment #8708578 - Attachment is obsolete: true
(Assignee)

Comment 9

17 days 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

17 days 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

16 days 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

15 days 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

10 days 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

10 days 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

10 days 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: 9 days 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

4 days ago
Depends on: 1374462
(Assignee)

Comment 18

2 days 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

a day 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

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

Comment 21

23 hours 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

23 hours ago
Try run for GETELEM_SUPER (and GETPROP_SUPER included): https://treeherder.mozilla.org/#/jobs?repo=try&revision=663279749a8827989af672c1a49813b9892351ad&group_state=expanded
You need to log in before you can comment on or make changes to this bug.