1169746 - Make SuperCall work in the JITs

Status: RESOLVED FIXED

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

Comment 1

[Hannes Verschore [:h4writer]]

Realistically this will be for next release.

Comment 2

[Ted Campbell [:tcampbell]]

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.

Comment 3

[Ted Campbell [:tcampbell]]

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.

Comment 4

[Ted Campbell [:tcampbell]]

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.

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: 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.

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: 0002-Bug-1169746-Support-super-in-Baseline.patch

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

Comment 7

[Ted Campbell [:tcampbell]]

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.

Comment 8

[Jan de Mooij [:jandem]]

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/

Comment 9

[Jan de Mooij [:jandem]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/702cf0215269
Backed out changeset a40056f67040 for breaking six-speed benchmark

Comment 12

[Ted Campbell [:tcampbell]]

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.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This backout also appears to have fixed this frequent failure: https://treeherder.mozilla.org/logviewer.html#?job_id=107137469&repo=mozilla-inbound

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/d29737d61c7a

Comment 15

[Ted Campbell [:tcampbell]]

Attachment: 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.

Comment 16

[Ted Campbell [:tcampbell]]

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 17

[Jan de Mooij [:jandem]]

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 :)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Ted Campbell [:tcampbell]]

Attachment: 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.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/777581283aee
https://hg.mozilla.org/mozilla-central/rev/68c84d4736ca

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aabaa4a353ef
Handle JSOP_SPREADSUPERCALL correctly in ICCall_Native. r=sstangl

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/aabaa4a353ef

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Quite an improvement! https://treeherder.mozilla.org/perf.html#/alerts?id=7385

Comment 24

[Ted Campbell [:tcampbell]]

Closing as FIXED since baseline support seems to have stuck. Address Ion support is future bugs.

Comment 25

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.