Make SuperCall work in the JITs

Assigned to



JavaScript Engine: JIT
2 years ago
5 days ago


(Reporter: efaust, Assigned: tcampbell)


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

leave-open, perf
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)



(4 attachments)

Comment hidden (empty)


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


19 days ago
Assignee: nobody → tcampbell

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.


19 days ago
Depends on: 1351951

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

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.

Comment 5

17 days ago
Created attachment 8875482 [details] [diff] [review]

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)

Comment 6

17 days ago
Created attachment 8875484 [details] [diff] [review]

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

Comment 7

16 days ago
Comment on attachment 8875484 [details] [diff] [review]

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]

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]

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


::: 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
Disable JSOP_SUPERCALL in IonBuilder. r=jandem
Support |super()| in Baseline. r=jandem

Comment 11

10 days ago
Backout by
Backed out changeset a40056f67040 for breaking six-speed benchmark

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:
Last Resolved: 9 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56


9 days ago
Keywords: leave-open
Resolution: FIXED → ---

Comment 15

9 days ago
Created attachment 8878199 [details] [diff] [review]

|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:
Try Mochitest-1:

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)


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

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.

Comment on attachment 8878199 [details] [diff] [review]

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
Don't use templateObjects for derived constructor calls. r=jandem
Support |super()| in Baseline. r=jandem


8 days ago
Blocks: 1177735

Comment 19

8 days ago
Created attachment 8878703 [details] [diff] [review]

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

Comment 21

8 days ago
Pushed by
Handle JSOP_SPREADSUPERCALL correctly in ICCall_Native. r=sstangl
Quite an improvement!
You need to log in before you can comment on or make changes to this bug.