JSPI Fuzzing: Interleave executions #7226
Conversation
| if (ordering & 1) { | ||
| // Defer it for later. Reuse the existing task for simplicity. | ||
| console.log(`(jspi: defer ${task.name})`); | ||
| task.func = /* async */ () => { |
There was a problem hiding this comment.
Since this is all in if (JSPI), do the async and await need to be comments?
There was a problem hiding this comment.
My (perhaps overly paranoid) approach was that when JSPI is off, we don't want the JS VM to see any async stuff, which could affect codegen. Like maybe they lay out the stack or basic blocks differently in async functions?
There was a problem hiding this comment.
I would hope these isolated awaits/asyncs would not affect other code. Anway, I'm fine with leaving them since it's consistent with all the other uses.
| console.log(`(jspi: defer ${task.name})`); | ||
| task.func = /* async */ () => { | ||
| console.log(`(jspi: finish ${task.name})`); | ||
| return /* await */ result; |
There was a problem hiding this comment.
Should this await be in a try/catch as well? If the suspended Wasm ends up throwing an error, that error would propagate out here IIUC.
There was a problem hiding this comment.
I believe the try-catch on 468 is enough? This code ends up run in that try-catch, so yes, an exception would propagate, but just to that try-catch, where it is caught and handled. (Note that we can't really add any handling for it here - all we could do is log and rethrow?)
scripts/fuzz_shell.js
Outdated
| // promises unresolved until later, which lets us interleave them. Note we | ||
| // never defer a task more than once (which would be pointless), and we |
There was a problem hiding this comment.
Deferring a task more than once would be interesting if it were deferred via chaining to another promise. And it would be even more interesting if it were "chained" to another promise by being returned from a future JSPI import call.
There was a problem hiding this comment.
Ah, interesting, what do you mean by "chaining" here? Is there an API for that?
There was a problem hiding this comment.
var newPromise = oldPromise.then((x) => { return x; }); or similar ways of wrapping the contents of the old promise in a new promise that will only be resolved after the old promise.
There was a problem hiding this comment.
Do you think that would test anything on the wasm side? I'd guess that just defers any wasm work further? I don't really know the VM side though.
As for chaining to a future JSPI call, that does happen already, if there is this in the wasm:
sleep();
sleep();
sleep();
Each one sleeps, then we continue, then we sleep again etc., which I think achieves the same?
There was a problem hiding this comment.
Yeah, maybe chaining in pure JS wouldn't add much. I think chaining through future JSPI calls might still be interesting because it would mix up the temporal order of tasks. The common case is that a task will not be resumed until a subsequently started task is completed, as in the sequence of sleeps above, but it is also interesting to test tasks that will not be resumed until prior tasks are completed.
test/lit/d8/fuzz_shell_jspi.wast
Outdated
| ;; RUN: echo "JSPI = 1;" > %t.js | ||
|
|
||
| ;; Use |echo| here to avoid quoting issues on windows paths. | ||
| ;; RUN: echo %S | node -e "process.stdout.write(require('fs').readFileSync(path.join(require('fs').readFileSync(0, 'utf-8'), '..', '..', '..', 'scripts', 'fuzz_shell.js'), 'utf-8').replace(/[/][*] async [*][/]/g, 'async').replace(/[/][*] await [*][/]/g, 'await'))" >> %t.js |
There was a problem hiding this comment.
Can this go in a helper script of some kind? At the very least, could we split the logic across multiple RUN lines?
There was a problem hiding this comment.
heh, yeah, this is a pretty horrible line. It was a hard-fought battle to even get it to work across all platforms!
I did some more work to simplify it now, it turns out lit has cat support (at least on linux... we'll see on CI for windows). If that works, I don't think we can split any more.
|
@brendandahl any more thoughts/ideas here? |
It's not really related to this PR, but the only other thing I can think of is having an import that sometimes return a value synchronously (i.e. not even through async function) and sometimes a promise. This really shouldn't behave differently now that v8 always converts the return value to a promise, so I'm not sure it's worth testing. |
|
Thanks @brendandahl , good idea, I'll look into that in a later PR. |
Rather than always do
await export()
now we might stash the Promise on the side, and execute it later, after
other stacks are executed and perhaps also saved.
To do this, rewrite the logic for calling the exports in a more flexible
manner. (That required altering the random seed in
fuzz_shell_orders.wast,to preserve the current order it was emitting.)
We do not fuzz with top-level await, so the output here looks a bit out
of order, but it does still end up with interleaved executions, which I think
is useful for fuzzing. @brendandahl maybe there is a better idea here though?