Closed Bug 1609889 Opened 5 years ago Closed 5 years ago

Separate wasm return values from calls in inlined Ion-to-Wasm calls

Categories

(Core :: JavaScript: WebAssembly, enhancement, P2)

Product:
Core
Component:
JavaScript: WebAssembly
Type:
enhancement

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: wingo, Assigned: wingo)

References

Details

Attachments

(1 obsolete file)

Related to bug 1608121.

When we use Ion and WebAssembly, we might get multiple return values from function calls. This patch refactors the direct JS-to-Wasm-via-Ion call path to better support multiple values.

The patch adds a safepoint at the call, as appears to be necessary in general, and also when results are captured as MIR values. There is a slight optimization in that void-returning Wasm functions just get a MConstant result that's visible to the optimizer.

Related to bug 1608121.

When we use Ion and WebAssembly, we might get multiple return values
from function calls. This patch refactors the direct JS-to-Wasm-via-Ion
call path to better support multiple values.

The patch adds a safepoint at the call, as appears to be necessary in
general, and also when results are captured as MIR values. There is a
slight optimization in that void-returning Wasm functions just get a
MConstant result that's visible to the optimizer.

Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/18f02f6b7243 Separate wasm return values from calls in inlined Ion-to-Wasm calls r=lth

Thanks! Here's that test case factored out, fwiw. Run as js --ion-eager --no-threads -f /tmp/foo.js.

var invalidatedFunction = function() { return false; }

var counter = 0;

function maybeInvalidate(iplusk) {
    if (iplusk === 0) {
        // This should happen only once; it will invalidate the call frame of
        // the ion() function, which should rewind to just after the wasm call
        // (if it got its own resume point).
        // Before the patch, the wasm call doesn't get its resume point, so
        // it's repeated and the importedFunc() is called once too many.
        counter++;
        invalidatedFunction = function () { return true; };
    }
}

function importedFunc(k) {
    for (let i = 100; i --> 0 ;) {
        maybeInvalidate(i + k);
    }
}

let { exports } = new WebAssembly.Instance(
    new WebAssembly.Module(
        wasmTextToBinary(`
        (module
          (func $imp (import "env" "importedFunc") (param i32))
          (func (export "exp") (param i32)
              local.get 0
              call $imp))`)
    ), {
        env: {
            importedFunc
        }
    }
);

function ion(k) {
    exports.exp(k);
    invalidatedFunction();
}

for (let k = 100; k --> 0 ;) {
    ion(k);
}

if (counter !== 1) { throw new Error; }

Result:

$ /home/wingo/src/mozilla-unified/+js-debug/dist/bin/js --ion-eager --no-threads -f /tmp/foo.js
Assertion failure: exprStack == stackDepth, at /home/wingo/src/mozilla-unified/js/src/jit/Recover.cpp:103
Segmentation fault (core dumped)
Flags: needinfo?(wingo)
Pushed by ccoroiu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/648a8170fec2 Separate wasm return values from calls in inlined Ion-to-Wasm calls r=lth

Backed out changeset 10909dde7e43 (Bug 1593886) for causing browser-chrome failures at toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=286741177&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=648a8170fec2e570748b9603f7cb16eb84025155

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=286733033&repo=autoland&lineNumber=50315

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=286733033&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=5faeb3565712e6861a3582778521bcc55d43f569

[task 2020-01-28T14:18:39.612Z] TEST-PASS | js/src/jit-test/tests/wasm/gc/stackmaps3.js | Success (code 0, args "--wasm-gc") [2.1 s]
[task 2020-01-28T14:18:40.708Z] Failed on list: 
[task 2020-01-28T14:18:40.708Z] [
[task 2020-01-28T14:18:40.708Z] 149520
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149521
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149522
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149523
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149524
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149525
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149526
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149527
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149528
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149529
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149530
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149531
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149532
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149533
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149534
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149535
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149536
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149537
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149538
[task 2020-01-28T14:18:40.708Z] ,
[task 2020-01-28T14:18:40.708Z] 149539
[task 2020-01-28T14:18:40.709Z] ,
[task 2020-01-28T14:18:40.709Z] ]
[task 2020-01-28T14:18:40.709Z] /builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/gc/stackmaps3.js:200:13 Error: Assertion failed: got false, expected true
[task 2020-01-28T14:18:40.709Z] Stack:
[task 2020-01-28T14:18:40.709Z]   @/builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/gc/stackmaps3.js:200:13
[task 2020-01-28T14:18:40.709Z] Exit code: 3
[task 2020-01-28T14:18:40.709Z] FAIL - wasm/gc/stackmaps3.js
[task 2020-01-28T14:18:40.709Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/gc/stackmaps3.js | /builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/gc/stackmaps3.js:200:13 Error: Assertion failed: got false, expected true (code 3, args "--wasm-compiler=baseline") [1.6 s]
[task 2020-01-28T14:18:40.709Z] INFO exit-status     : 3
[task 2020-01-28T14:18:40.709Z] INFO timed-out       : False
[task 2020-01-28T14:18:40.709Z] INFO stdout          > Failed on list:
[task 2020-01-28T14:18:40.709Z] INFO stdout          > [
[task 2020-01-28T14:18:40.709Z] INFO stdout          > 149520
[task 2020-01-28T14:18:40.709Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.709Z] INFO stdout          > 149521
[task 2020-01-28T14:18:40.709Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.710Z] INFO stdout          > 149522
[task 2020-01-28T14:18:40.710Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.710Z] INFO stdout          > 149523
[task 2020-01-28T14:18:40.710Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.710Z] INFO stdout          > 149524
[task 2020-01-28T14:18:40.710Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.710Z] INFO stdout          > 149525
[task 2020-01-28T14:18:40.710Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.710Z] INFO stdout          > 149526
[task 2020-01-28T14:18:40.710Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.710Z] INFO stdout          > 149527
[task 2020-01-28T14:18:40.710Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.711Z] INFO stdout          > 149528
[task 2020-01-28T14:18:40.711Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.711Z] INFO stdout          > 149529
[task 2020-01-28T14:18:40.711Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.711Z] INFO stdout          > 149530
[task 2020-01-28T14:18:40.711Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.711Z] INFO stdout          > 149531
[task 2020-01-28T14:18:40.711Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.711Z] INFO stdout          > 149532
[task 2020-01-28T14:18:40.711Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.711Z] INFO stdout          > 149533
[task 2020-01-28T14:18:40.712Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.712Z] INFO stdout          > 149534
[task 2020-01-28T14:18:40.712Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.712Z] INFO stdout          > 149535
[task 2020-01-28T14:18:40.712Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.712Z] INFO stdout          > 149536
[task 2020-01-28T14:18:40.712Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.712Z] INFO stdout          > 149537
[task 2020-01-28T14:18:40.712Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.712Z] INFO stdout          > 149538
[task 2020-01-28T14:18:40.712Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.712Z] INFO stdout          > 149539
[task 2020-01-28T14:18:40.712Z] INFO stdout          > ,
[task 2020-01-28T14:18:40.713Z] INFO stdout          > ]
[task 2020-01-28T14:18:40.713Z] INFO stderr         2> /builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/gc/stackmaps3.js:200:13 Error: Assertion failed: got false, expected true
[task 2020-01-28T14:18:40.713Z] INFO stderr         2> Stack:
[task 2020-01-28T14:18:40.713Z] INFO stderr         2> @/builds/worker/workspace/build/src/js/src/jit-test/tests/wasm/gc/stackmaps3.js:200:13
Flags: needinfo?(wingo)
Backout by dvarga@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6afd2146027b Backed out changeset 648a8170fec2 for causing spiderMonkey buil failure at src/jit-test/tests/wasm/gc/stackmaps3.js
Flags: needinfo?(wingo)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:wingo, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(wingo)

Okeysmokey, I finally managed to reproduce the failure with a gczeal incantation, and reduced the test case to:

// |jit-test| skip-if: !wasmReftypesEnabled()

// Allocate heap objects via a trampoline through WebAssembly.  We
// trigger compacting GC every few allocations, so this tests that
// intermediate results are properly relocated.

function Pair(head, tail) {
    this.head = head;
    this.tail = tail;
}

let counter = 0;
let i = wasmEvalText(`
    (module
      (func $consNextInt (import "imports" "consNextInt")
        (result anyref))

      (func $makePair (export "makePair") (result anyref)
        call $consNextInt))`,
    {imports: {consNextInt: () => new Pair(counter++, null)}});

// Run a compacting GC every 50 allocations.
gczeal(14, 50);

for (let n = 0; n < 200000; n++) {
    assertEq(counter, n);
    let p = i.exports.makePair();
    assertEq(p.head, n)
    assertEq(p.tail, null);
}

// If we get here, the test finished successfully.

I think it's a bit odd because there shouldn't be any GC while any anyref value is live on the WebAssembly stack. But maybe we are triggering GC on JS-to-WebAssembly edges for some reason, and in that case indeed the stack map wouldn't include the return register... Humm!

Flags: needinfo?(wingo)

Here's a version that's runnable directly, via e.g. dist/bin/js --wasm-gc --no-threads --wasm-compiler=baseline

function Pair(head, tail) {
    this.head = head;
    this.tail = tail;
}

let counter = 0;
let i = new WebAssembly.Instance(
    new WebAssembly.Module(wasmTextToBinary(`
      (module
        (func $consNextInt (import "imports" "consNextInt")
          (result anyref))

        (func $makePair (export "makePair") (result anyref)
          call $consNextInt))`)),
    {imports: {consNextInt: () => new Pair(counter++, null)}});

// Run a compacting GC every 50 allocations.
gczeal(14, 50);

for (let n = 0; n < 200000; n++) {
    let p = i.exports.makePair();
    if (p.head !== n) throw new Error('unexpected: ' + p.head + ' vs ' + n);
}
Attachment #9121479 - Attachment is obsolete: true

Sadly, abandoning this rev, as mentioned in phab. Boo!

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: