Closed
Bug 1169746
Opened 9 years ago
Closed 7 years ago
Make SuperCall work in the JITs
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: efaust, Assigned: tcampbell)
References
(Blocks 3 open bugs)
Details
(Keywords: perf)
Attachments
(4 files)
2.18 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
15.19 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
1.53 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•8 years ago
|
Blocks: sm-js-perf
Priority: -- → P2
Updated•8 years ago
|
Priority: P2 → P1
Updated•8 years ago
|
Priority: P2 → P1
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tcampbell
Assignee | ||
Comment 2•7 years 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 | ||
Comment 3•7 years 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: ares-6
Assignee | ||
Comment 4•7 years 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•7 years ago
|
||
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•7 years ago
|
||
Support super calls in baseline. Includes the associated opcodes to allow practical code to compile.
Assignee | ||
Comment 7•7 years 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 8•7 years ago
|
||
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 9•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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
Comment 14•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 15•7 years ago
|
||
|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•7 years 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•7 years 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 17•7 years ago
|
||
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•7 years 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
Assignee | ||
Comment 19•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8878703 -
Flags: review+
Comment 20•7 years ago
|
||
bugherder |
Comment 21•7 years ago
|
||
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aabaa4a353ef
Handle JSOP_SPREADSUPERCALL correctly in ICCall_Native. r=sstangl
Comment 22•7 years ago
|
||
bugherder |
Comment 23•7 years ago
|
||
Quite an improvement! https://treeherder.mozilla.org/perf.html#/alerts?id=7385
Assignee | ||
Comment 24•7 years ago
|
||
Closing as FIXED since baseline support seems to have stuck. Address Ion support is future bugs.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 25•7 years ago
|
||
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.
Description
•