Closed Bug 1169746 Opened 9 years ago Closed 7 years ago

Make SuperCall work in the JITs


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




Tracking Status
firefox56 --- fixed


(Reporter: efaust, Assigned: tcampbell)


(Blocks 3 open bugs)


(Keywords: perf)


(4 files)

      No description provided.
Blocks: 1167472
Blocks: sm-js-perf
Priority: -- → P2
Keywords: perf
Priority: P2 → P1
Realistically this will be for next release.
Priority: P1 → P2
Priority: P2 → P1
Assignee: nobody → 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.
Depends on: 1351951
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: ares-6
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.
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)
Support super calls in baseline. Includes the associated opcodes to allow practical code to compile.
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+
Pushed by
Disable JSOP_SUPERCALL in IonBuilder. r=jandem
Support |super()| in Baseline. r=jandem
Backout by
Backed out changeset a40056f67040 for breaking six-speed benchmark
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.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Keywords: leave-open
Resolution: FIXED → ---
|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)
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
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+
Pushed by
Don't use templateObjects for derived constructor calls. r=jandem
Support |super()| in Baseline. r=jandem
Blocks: six-speed
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+
Pushed by
Handle JSOP_SPREADSUPERCALL correctly in ICCall_Native. r=sstangl
Closing as FIXED since baseline support seems to have stuck. Address Ion support is future bugs.
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.