1541708 - Jetstream3 ML does polymorphic calls

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Denis Palmeiro [:denispal]]

The "ML" test in Jetstream2 performs very poorly in Firefox compared to Chrome:

Firefox:
ML - 20.986
First - 17.361, Worst - 21.810, Average - 24.410

Chrome:
ML - 55.008
First - 27.174, Worst - 69.930, Average - 87.589

Profile is here: https://perfht.ml/2UfwPxP

Ion compilation aborts early due to:

[IonAbort] Unsupported opcode: checkreturn
[IonAbort] aborted @ file:///home/denis/src/JetStream2/ARES-6/ml/index.js
[IonAbort] Disabling Ion compilation of script file:///home/denis/src/JetStream2/ARES-6/ml/index.js:4554:19

On top of that, I see over 40,000 IC fallbacks in the same function almost all on "NewArray".

Comment 1

[Mayank Bansal]

Is this still an issue?

Comment 2

[Iain Ireland [:iain]]

We support JSOp::CheckReturn in Ion now, and I don't see any NewArray fallbacks.

We're still significantly slower, though. Taking a quick look at profiles, one thing that stands out is that the two get calls in mmul aren't inlined because they're both polymorphic. As far as I can tell, it's genuine polymorphism where we call two different functions.

One is Matrix.get:

        get(rowIndex, columnIndex) {
            return this[rowIndex][columnIndex];
        }

and the other is MatrixTransposeView.get:

        get(rowIndex, columnIndex) {
            return this.matrix.get(columnIndex, rowIndex);
        }

V8 does polymorphic inlining. This doesn't show up in comparison reports, because we disable inlining there, but it probably hurts us quite a bit.

Comment 3

[Mayank Bansal]

Attachment: WIP: Bug 1541708 - Part 1: Record concrete callee for script-keyed call guards

emitCalleeGuard keys later call-IC stubs on the callee's BaseScript
(lambda-clone tolerance), so the concrete JSFunction is not recoverable
from stub data. Record it with a LoadObject op next to the guard so
polymorphic trial inlining can key a dispatch on function identity.
Also gate that feature behind javascript.options.polymorphic_call_inlining.

Comment 4

[Mayank Bansal]

Attachment: WIP: Bug 1541708 - Part 2: Polymorphic trial inlining

Trial inlining previously skipped call sites with more than one live IC
stub ("SKIP: Polymorphic"). Inline up to three variants instead: rank
the live scripted-call stubs by entered count, apply the existing
inlining decision to each, and rewrite every selected stub in place with
its own child ICScript. Children are deduped by target script and keyed
per (pcOffset, script) so bailout reconstruction can find the right one.

Attaching a variant to an already-inlined site keeps the site's existing
variants instead of tearing the inlining down; a failure mid-sequence
keeps the successfully attached prefix. Selection requires the chosen
variants to cover at least 90 percent of the site's executions.

Comment 5

[Mayank Bansal]

Attachment: WIP: Bug 1541708 - Part 3: Inline polymorphic calls in Warp

Warp previously refused call sites with multiple live IC stubs and
emitted a generic MCall. When trial inlining has rewritten two or more
stubs at a site, the oracle now snapshots one variant per stub
(WarpPolyInlineCall, deduped by child ICScript) and WarpBuilder emits an
explicit dispatch: MStrictConstantCompareObject against each known
callee, one inlined body per match, and a generic call as the fallback,
merged with a Phi.

A variant whose target fails to snapshot is isolated: only its stub is
unlinked and its script marked uninlineable; other variants compile.

Comment 6

[Iain Ireland [:iain]]

When I finally clear some space on my plate I want to do a big inlining rewrite; any changes in this direction would be better based on the new version.

Comment 7

[Mayank Bansal]

Attachment: WIP: Bug 1541708 - Fix build: restore maybeInlineGetter signature, drop unused pcOffset

An edit inserting the polymorphic path consumed the maybeInlineGetter
function signature; restore it. Also drop a dead pcOffset variable
(-Werror,-Wunused-variable on CI).

Comment 8

[Mayank Bansal]

Attachment: WIP: Bug 1541708 - Fix build: OffthreadGCPtr is not assignable

Fill WarpPolyInlineCall variants through OffthreadGCPtr::init instead of
copy-assigning Variant structs; give the struct default member
initializers so the array elements are constructed.

Comment 9

[Mayank Bansal]

Attachment: WIP: Bug 1541708 - Part 1: Record the concrete callee for script-keyed call guards. ?

emitCalleeGuard keys later call-IC stubs on the callee's BaseScript so that
lambda clones share a stub. That leaves the concrete JSFunction unrecoverable
from stub data, which polymorphic trial inlining needs in order to key a Warp
dispatch on callee identity.

Record it with a new MetaScriptedCallee op emitted next to the guard. Like
MetaCreateThis, this op generates no code in any compiler; it exists only to
carry a stub field. Using an executable op such as LoadObject would cost a
register and a stub-field load in every script-keyed scripted-call stub in the
engine, because the Baseline CacheIR compiler does not eliminate dead ops.

Also add javascript.options.polymorphic_call_inlining, used by the later parts.

Comment 10

[Mayank Bansal]

Attachment: WIP: Bug 1541708 - Part 2: Polymorphic trial inlining. ?

Trial inlining skipped any call site with more than one live IC stub. Inline up
to three callees instead: rank the live scripted-call stubs by their own hit
counts, apply the existing inlining decision to each, and rewrite the site with
one stub per variant, each with its own child ICScript. Children are deduped by
target script and keyed per (pcOffset, script) so bailout reconstruction finds
the right one.

A site that has already been inlined and then sees a second callee previously
moved to TrialInliningState::Failure, which is terminal, so it could never be
reconsidered. Such a site now moves to the new PolymorphicCandidate state
instead, which is what makes this feature reachable at all. The polymorphic
path moves the site to Failure itself once it decides the callees are too
spread out, so the number of re-decisions stays bounded.

Notes on the counting: entered counts are incremented on stub entry, not on
success, so they are cumulative down the chain and a stub's own hits are
enteredCount(i) - enteredCount(i + 1). Admission therefore uses each stub's own
hits rather than its cumulative count, or a rare callee at a hot site would
clear the entry threshold on traffic belonging to the stubs ahead of it. For
the same reason the site total is the first stub's entered count on its own:
because ResetEnteredCounts clears the whole chain including the fallback on
every attach, that count already includes the executions that fell through.

Variants are attached rarest first. addNewStub prepends, so the chain ends up
ordered most-frequent first, which is the order Baseline checks them in and the
order WarpOracle later walks.

The target script recorded for each child is a GC pointer held in a container
that is not barriered, so ICScript::trace traces it manually and
removeInlinedChild emits the matching pre-barrier. Without that it would go
stale after a compacting GC and findInlinedChild would crash or match the wrong
child.

Comment 11

[Mayank Bansal]

Attachment: WIP: Bug 1541708 - Part 3: Inline polymorphic calls in Warp. ?

Warp refused call sites with more than one live IC stub and emitted a generic
MCall. When trial inlining has rewritten two or more stubs at a site, the
oracle now snapshots one variant per stub (WarpPolyInlineCall, deduped by child
ICScript) and WarpBuilder emits an explicit dispatch: an
MStrictConstantCompareObject against each known callee, one inlined body per
match, and a generic call as the fallback, merged with a phi. Because trial
inlining leaves the stub chain ordered most-frequent first, the first test is
the likeliest callee.

buildInlinedCall is split so the body-building half (buildInlinedCallCore) can
be reused per variant.

buildCallOp has already consumed the operands into its CallInfo by the time the
polymorphic path runs, so the callee is taken from there and each arm gets its
own CallInfo populated by the new CallInfo::initFrom rather than popping the
stack again. The join block replaces the per-arm result inherited by
inheritSlots with the merged phi instead of pushing on top of it, matching what
buildInlinedCall does with the callee it leaves on the stack.

A variant whose target fails to snapshot is isolated: only its stub is
unlinked, its script is marked uninlineable, and the other variants still
compile.