1660862 - Warp: GuardSpecificFunction captures stale function flags for lazy Wasm stubs

Status: RESOLVED FIXED

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

Description

[Iain Ireland [:iain]]

Discovered while working on bug 1660592:
wasm/ion-lazy-tables.js and wasm/regress/ion-lazy-stubs.js fail when run with --warp --no-threads --baseline-warmup-threshold=0 --ion-warmup-threshold=10.

The issue is that the wasmWithJitEntry flag isn't set until the first time we call a wasm function. So if we capture the flags in GuardSpecificFunction before performing the actual call, the flags are out of date when we transpile. Then we assert while creating the WrappedFunction. --no-threads is necessary to trigger the failure because the failing assertion is only executed on the main thread. When the assertion is disabled, the testcase passes. It looks like we end up calling the function via C++ through WasmCall, but I don't know the wasm code very well. If that's true, this is just a performance regression, not a correctness issue.

Comment 1

[Jan de Mooij [:jandem]]

This is pretty interesting: Wasm sets the function's WasmJitEntry for certain exports lazily, but that means if the export is first called from a Baseline IC we optimize it as native call instead of a fast JIT ABI call (because hasJitEntry is still false).

I have a patch to set the WasmJitEntry to the JIT interpreter trampoline (just a call into the VM). It fixes this, with --no-ion for the micro-benchmark below:

before: 1092 ms
after:   157 ms

function wasmEvalText(str) {
  let binary = wasmTextToBinary(str);
  let m = new WebAssembly.Module(binary);
  return new WebAssembly.Instance(m);
}
function test() {
    var table = wasmEvalText(`(module
        (func $add (param i32) (param i32) (result i32)
          (i32.add (get_local 0) (get_local 1))
        )
        (table (export "table") 10 funcref)
        (elem (i32.const 0) $add)
    )`).exports.table;

    for (var i = 0; i < 20; i++) {} // Warmup

    var exp = table.get(0);
    var t = dateNow();
    var res = 0;
    for (var i = 0; i < 10_000_000; i++) {
      res = exp(i, 1);
    }
    print(dateNow() - t);
    print(res);
}
test();

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1660862 - Use the JIT's interpreter-trampoline for Wasm exports with lazy stubs. r?bbouvier!

This ensures isWasmWithJitEntry() is always true for the function, fixing a perf
cliff with ICs (function was called with native ABI instead of JIT ABI). It also
fixes an assertion failure when Warp is enabled.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d530fda38c2
Use the JIT's interpreter-trampoline for Wasm exports with lazy stubs. r=bbouvier

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4d530fda38c2