Closed Bug 1554080 Opened 5 months ago Closed 5 months ago

Move jitCodeSkipArgCheck_ to JitScript

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

There are some benefits here:

a) It will remove a word from JSScript (worth > 20 KB in the browser IIRC) and corresponding (same layout) Wasm imports.
b) Because this field depends on JitScript lifetime, moving it to JitScript makes sense and might even help catch bugs.

Now, masm.loadJitCodeNoArgCheck has a few callers:

  1. Wasm's GenerateImportJitExit. To avoid an extra dependent load here, we can store JitScript* instead of BaselineScript* in FuncImportTls and then load that directly. This is also nice because it means we will get Baseline Interpreter support and also the exit entry can persist when the BaselineScript (but not the JitScript) is destroyed.

  2. Ion's CallGeneric and CallKnown. For CallKnown we can probably bake in the JitScript* pointer directly, per (b) above, and for CallGeneric the extra load should be negligible.

Type: defect → task
Type: task → enhancement

(In reply to Jan de Mooij [:jandem] from comment #0)

  1. For CallKnown we can probably bake in the JitScript* pointer directly, per (b) above, and for CallGeneric the extra load should be negligible.

I tried this CallKnown optimization but I don't see any perf difference on micro-benchmarks + Octane/Kraken with --ion-inlining=off. Given that I'll punt on this because it does add some complexity (and a pointer in MCall).

This removes a word from JSScript and corresponding Wasm data structures.

Furthermore, the skip-argument-type-checks optimization depends on JitScript's
lifetime so moving it to JitScript feels right and might help catch bugs.

The lazy link stub used to assume a JIT caller but this is no longer the case.
We can then fold clearDependentWasmImports into unlinkDependentWasmImports.

Depends on D32441

This has a few benefits:

  • Less toggling when we discard/reallocate the BaselineScript without discarding
    the JitScript.

  • Wasm's JIT exit optimization will work with the Baseline Interpreter in the
    future.

  • The next part will use this to eliminate a load.

Depends on D32443

This way we have two loads instead of three.

Depends on D32444

Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e66b838b3b6
part 1 - Move jitCodeSkipArgCheck_ from JSScript to JitScript. r=bbouvier,iain
https://hg.mozilla.org/integration/autoland/rev/e98c9d639879
part 2 - Allow Wasm FFI calls to lazy link stub. r=bbouvier
https://hg.mozilla.org/integration/autoland/rev/e8484107b29e
part 3 - Use UniquePtr for BaselineScript::dependentWasmImports_. r=bbouvier
https://hg.mozilla.org/integration/autoland/rev/81d0d9e9990d
part 4 - Store JitScript* instead of BaselineScript* in FuncImportTls. r=bbouvier
https://hg.mozilla.org/integration/autoland/rev/b32580fad048
part 5 - Replace the masm.loadJitCodeNoArgCheck in GenerateImportJitExit with a TLS load. r=bbouvier

== Change summary for alert #21270 (as of Tue, 04 Jun 2019 05:07:53 GMT) ==

Improvements:

0% Base Content JS linux64-shippable-qr opt 3,999,124.00 -> 3,986,694.67
0% Base Content JS linux64-shippable opt 3,998,876.00 -> 3,986,614.67
0% Base Content JS macosx1010-64-shippable opt 4,000,038.67 -> 3,987,648.00
0% Base Content JS linux64-shippable opt 3,998,924.00 -> 3,986,945.33
0% Base Content JS windows10-64-shippable-qr opt 4,055,501.33 -> 4,043,562.67
0% Base Content JS windows10-64-shippable opt 4,055,281.33 -> 4,043,406.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=21270

You need to log in before you can comment on or make changes to this bug.