Make SuperCall work in the JITs

REOPENED
Assigned to

Status

()

Core
JavaScript Engine: JIT
P1
normal
REOPENED
2 years ago
5 days ago

People

(Reporter: efaust, Assigned: tcampbell)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs, {leave-open, perf})

unspecified
mozilla56
leave-open, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(4 attachments)

Comment hidden (empty)
(Reporter)

Updated

2 years ago
Blocks: 1167472
Blocks: 1307062
Priority: -- → P2
Keywords: perf
Priority: P2 → P1
Realistically this will be for next release.
Priority: P1 → P2
Priority: P2 → P1
(Assignee)

Updated

19 days ago
Assignee: nobody → tcampbell
(Assignee)

Comment 2

19 days ago
SuperCall opcodes are already partially supported in the JITs. They currently do not run because associated opcodes such as JSOP_SUPERFUN are missing. The bailout code for supercalls should be reviewed. Bug 1351951 already identified latent bugs, but further testing is needed once this is enabled.
(Assignee)

Updated

19 days ago
Depends on: 1351951
(Assignee)

Comment 3

19 days ago
The ARES-6 ML benchmark uses |super()| in constructor for |Matrix| type and as a result spends 90% of total runtime in interpreter constructing objects.
Blocks: 1304984
(Assignee)

Comment 4

18 days ago
Need to implement JSOP_SUPERFUN and the JSOP_CHECKTHIS variants.

IMPORTANT: This is existing support for SUPERCALL in the JITs, but it is not tested and has a number of existing defects. Once JSOP_SUPERFUN is added, this code will become live and causes problems. The SUPERCALL variants need further testing and review, particularly bailout and looking at cases that explicitly check for JSOP_NEW/SPREADNEW instead of a more general constructor call check.
(Assignee)

Comment 5

17 days ago
Created attachment 8875482 [details] [diff] [review]
0001-Bug-1169746-Disable-JSOP_SUPERCALL-in-IonBuilder.patch

I have found bugs related to this in bailout in past, and worry that will become unblocked without further testing of this existing code.

Currently not possible to write a script that uses this code because JSOP_SUPERFUN is missing.
Attachment #8875482 - Flags: review?(jdemooij)
(Assignee)

Comment 6

17 days ago
Created attachment 8875484 [details] [diff] [review]
0002-Bug-1169746-Support-super-in-Baseline.patch

Support super calls in baseline. Includes the associated opcodes to allow practical code to compile.
(Assignee)

Comment 7

16 days ago
Comment on attachment 8875484 [details] [diff] [review]
0002-Bug-1169746-Support-super-in-Baseline.patch

Support super calls in Baseline. Adds missing opcode support, and fixes small issues in existing call ICs.
Attachment #8875484 - Flags: review?(jdemooij)
Comment on attachment 8875482 [details] [diff] [review]
0001-Bug-1169746-Disable-JSOP_SUPERCALL-in-IonBuilder.patch

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

::: js/src/jit/IonBuilder.cpp
@@ +2330,5 @@
> +#ifdef DEBUG
> +      // Most of the infrastructure for these exists in Ion, but needs review
> +      // and testing before these are enabled. Once other opcodes that are used
> +      // in derived classes are supported in Ion, this can be bettered validated
> +      // with testcases. Pay special to bailout and other areas where JSOP_NEW

Nit: s/bettered/better/, s/special/special attention/
Attachment #8875482 - Flags: review?(jdemooij) → review+
Comment on attachment 8875484 [details] [diff] [review]
0002-Bug-1169746-Support-super-in-Baseline.patch

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

\o/

::: js/src/jit/BaselineCompiler.cpp
@@ +1433,5 @@
> +{
> +    frame.syncStack(0);
> +    masm.loadValue(frame.addressOfStackValue(frame.peek(-1)), R0);
> +
> +    return emitCheckThis(R0, /*reinit=*/true);

Nit: add some more spaces: /* reinit = */ true

@@ +1454,5 @@
> +    if (reinit) {
> +        if (!callVM(ThrowInitializedThisInfo))
> +            return false;
> +    }
> +    else {

Nit: } else {

@@ +4266,5 @@
> +    masm.loadObjProto(callee, proto);
> +
> +    // Use VMCall for missing or lazy proto
> +    Label needVMCall;
> +    masm.branchPtr(Assembler::BelowOrEqual, proto, ImmWord(1), &needVMCall);

Maybe add the following assert here (and in the SUPERBASE code) to ensure we change this when we change LazyProto:

MOZ_ASSERT(uintptr_t(TaggedProto::LazyProto) == 1);

@@ +4274,5 @@
> +
> +    // Use VMCall if not constructor
> +    masm.load16ZeroExtend(Address(proto, JSFunction::offsetOfFlags()), scratch);
> +    masm.and32(Imm32(JSFunction::CONSTRUCTOR), scratch);
> +    masm.branchTest32(Assembler::Zero, scratch, scratch, &needVMCall);

Can fuse the and32 and branchTest32:

masm.branchTest32(Assembler::Zero, scratch, Imm32(JSFunction::CONSTRUCTOR), &needVMCall);

Could even use branchTest32 with an address, see MacroAssembler::branchIfFunctionHasNoScript. We could add MacroAssembler::branchIfNotConstructor or something.

::: js/src/jit/BaselineCompiler.h
@@ +296,5 @@
>  
>    private:
>      MethodStatus emitBody();
>  
> +    MOZ_MUST_USE bool emitCheckThis(ValueOperand val, bool reinit=false);

Nit: spaces around =

::: js/src/jit/VMFunctions.cpp
@@ +1827,5 @@
>      return superBase;
>  }
>  
> +JSObject*
> +SuperFunOperation(JSContext* cx, HandleObject callee)

Please move this function to Interpreter.cpp/h and call it from the interpreter too.
Attachment #8875484 - Flags: review?(jdemooij) → review+

Comment 10

10 days ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d29737d61c7a
Disable JSOP_SUPERCALL in IonBuilder. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a40056f67040
Support |super()| in Baseline. r=jandem

Comment 11

10 days ago
Backout by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/702cf0215269
Backed out changeset a40056f67040 for breaking six-speed benchmark
(Assignee)

Comment 12

10 days ago
I had to backout the |super()| call patch for breaking six-speed. It turns out as soon as we support baseline, the ICs get used by Ion as template objects and this causes initialization problems.
This backout also appears to have fixed this frequent failure: https://treeherder.mozilla.org/logviewer.html#?job_id=107137469&repo=mozilla-inbound
https://hg.mozilla.org/mozilla-central/rev/d29737d61c7a
Status: NEW → RESOLVED
Last Resolved: 9 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

9 days ago
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
(Assignee)

Comment 15

9 days ago
Created attachment 8878199 [details] [diff] [review]
0003-Bug-1169746-Don-t-use-templateObjects-for-derived-co.patch

|IonBuilder::createThisScriptedBaseline| tries to fast path |js::jit::CreateThis|, but doesn't detected derived class constructor case.

This results in an object being instantiated before calling derived class constructor, when our current semantics are to pass an UninitializedLexical. In turn, the derived constructor fails it's JSOP_CHECKTHISREINIT test.

Try JS: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac29e45f6f93e64b37419e6c44958e1f6006075f
Try Mochitest-1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c664d6bbdd587e68c5e998d172a330f89374da4e

Leaving the templateObject in baseline until further optimization of Ion class support. The BaselineIC is in response to a JSOP_NEW and it seems hacky to inspect called script to determine if is derived.
Attachment #8878199 - Flags: review?(jdemooij)
(Assignee)

Updated

9 days ago
Attachment #8878199 - Attachment description: 0001-Bug-1169746-Don-t-use-templateObjects-for-derived-co.patch → 0003-Bug-1169746-Don-t-use-templateObjects-for-derived-co.patch
Attachment #8878199 - Attachment filename: 0001-Bug-1169746-Don-t-use-templateObjects-for-derived-co.patch → 0003-Bug-1169746-Don-t-use-templateObjects-for-derived-co.patch
(Assignee)

Comment 16

9 days ago
Looks like that try run was on a broken version of inbound. Merged a few more commits from inbound and try is green for tc(B) and the tc-M(1) intermittent from Comment 13 is also resolved. six-speed benchmark is also fixed.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=946744fa60fa69ab875b469fd5e9cb509155c2e2
Comment on attachment 8878199 [details] [diff] [review]
0003-Bug-1169746-Don-t-use-templateObjects-for-derived-co.patch

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

Oops, it's good six-speed caught it :)
Attachment #8878199 - Flags: review?(jdemooij) → review+

Comment 18

8 days ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/777581283aee
Don't use templateObjects for derived constructor calls. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c84d4736ca
Support |super()| in Baseline. r=jandem

Updated

8 days ago
Blocks: 1177735
(Assignee)

Comment 19

8 days ago
Created attachment 8878703 [details] [diff] [review]
0004-Bug-1169746-Handle-SUPERSPREADCALL-correctly-in-ICCa.patch

This fixes ICCall_Native to handle JSOP_SPREADSUPERCALL correctly. It currently adds a stub which fails to ever match because it guards wrong argument. This gives another 15% on ARES-6/ML.

See |ICCallScriptedCompiler::generateStubCode| vs |ICCall_Native::Compiler::generateStubCode|.

The newTarget argument wasn't considered under case where it is both super and spread.
Attachment #8878703 - Flags: review+

Comment 20

8 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/777581283aee
https://hg.mozilla.org/mozilla-central/rev/68c84d4736ca

Comment 21

8 days ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aabaa4a353ef
Handle JSOP_SPREADSUPERCALL correctly in ICCall_Native. r=sstangl
https://hg.mozilla.org/mozilla-central/rev/aabaa4a353ef
Quite an improvement! https://treeherder.mozilla.org/perf.html#/alerts?id=7385
You need to log in before you can comment on or make changes to this bug.