[wasm] Add stack trace API test for Wasm-in-JS inlining [v8/v8 : main]
Daniel Lehmann (Gerrit)
Daniel Lehmann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Lehmann
| Auto-Submit | +1 |
1 comment
Finally getting around to a more in-depth test of Wasm-in-JS body inlining with V8's non-standard stack trace API. Could you please take a look?
Related details
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 1 comment![]( "Open in Gerrit")
// Flags: --turbolev --turbolev-inline-js-wasm-wrappers
// Flags: --turboshaft-wasm-in-js-inlining
// Flags: --allow-natives-syntax
// Flags: --trace-turbo-inliningIs this consistent with `wasm-gc-inlining-stacktrace-api.js` and the only meaningful change are the flags and the filecheck assertions that we are indeed inlining?
Related details
- Daniel Lehmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann added 1 comment![]( "Open in Gerrit")
// Flags: --turbolev --turbolev-inline-js-wasm-wrappers
// Flags: --turboshaft-wasm-in-js-inlining
// Flags: --allow-natives-syntax
// Flags: --trace-turbo-inliningIs this consistent with `wasm-gc-inlining-stacktrace-api.js` and the only meaningful change are the flags and the filecheck assertions that we are indeed inlining?
Oh, I hadn't looked at that (but apparently Gemini has). I let Gemini generate something and then iterated on it a couple of times to a) simplify and b) increase coverage of the stack trace API. Indeed, they are very similar, but the differences are:
- The new body inlining doesn't support several of the instructions in the old test, hence using array.len instead of struct.new and extern.convert_any etc. to trigger the trap.
- My test covers more of the API, e.g., getLineNumber, getPosition, the error type and `message` property, the number of frames and some frames above the Wasm and JS caller.
- My test is a filecheck test that ensures we actually inline.
- Your test also covers when the wasm call is inside a try-catch (which we don't inline atm).
Not sure, keep both or shall I instead extend yours + simply include it in my test?
Related details
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daniel Lehmann
| Auto-Submit | +0 |
5 comments
// Flags: --turbolev --turbolev-inline-js-wasm-wrappers
// Flags: --turboshaft-wasm-in-js-inlining
// Flags: --allow-natives-syntax
// Flags: --trace-turbo-inliningDaniel LehmannIs this consistent with `wasm-gc-inlining-stacktrace-api.js` and the only meaningful change are the flags and the filecheck assertions that we are indeed inlining?
Oh, I hadn't looked at that (but apparently Gemini has). I let Gemini generate something and then iterated on it a couple of times to a) simplify and b) increase coverage of the stack trace API. Indeed, they are very similar, but the differences are:
- The new body inlining doesn't support several of the instructions in the old test, hence using array.len instead of struct.new and extern.convert_any etc. to trigger the trap.
- My test covers more of the API, e.g., getLineNumber, getPosition, the error type and `message` property, the number of frames and some frames above the Wasm and JS caller.
- My test is a filecheck test that ensures we actually inline.
- Your test also covers when the wasm call is inside a try-catch (which we don't inline atm).
Not sure, keep both or shall I instead extend yours + simply include it in my test?
I decided to improve the old test, and reuse it in the new one (the `// Flags:` comments if imported files are not carried over by the test runner, which is good in this case). Also fixed the test failures.
// Flags: --no-always-sparkplug --expose-gcDrive-by: We never explicitly `gc()` here, so I removed that flag.
for (let i = 0; i < 10; ++i) {What's the reason for running 10 times before `OptimizeFunctionOnNextCall`? Doesn't the latter enforce that we tier-up and running once unoptimized is sufficient?
Error.prepareStackTrace = (error, frames) => {This function was duplicated, so I decided to pull it into the `testOptimized` helper instead of repeating the comments and assertions.
const globalNullArray = builder.addGlobal(wasmRefNullType(array), true, false);I changed the trapping Wasm function to `array.len` on a null array, since that's also supported by the new inlining, whereas `struct.new` was not.
Related details
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 6 comments![]( "Open in Gerrit")
// Flags: --turbolev --turbolev-inline-js-wasm-wrappers
// Flags: --turboshaft-wasm-in-js-inlining
// Flags: --allow-natives-syntax
// Flags: --trace-turbo-inliningDaniel LehmannIs this consistent with `wasm-gc-inlining-stacktrace-api.js` and the only meaningful change are the flags and the filecheck assertions that we are indeed inlining?
Daniel LehmannOh, I hadn't looked at that (but apparently Gemini has). I let Gemini generate something and then iterated on it a couple of times to a) simplify and b) increase coverage of the stack trace API. Indeed, they are very similar, but the differences are:
- The new body inlining doesn't support several of the instructions in the old test, hence using array.len instead of struct.new and extern.convert_any etc. to trigger the trap.
- My test covers more of the API, e.g., getLineNumber, getPosition, the error type and `message` property, the number of frames and some frames above the Wasm and JS caller.
- My test is a filecheck test that ensures we actually inline.
- Your test also covers when the wasm call is inside a try-catch (which we don't inline atm).
Not sure, keep both or shall I instead extend yours + simply include it in my test?
I decided to improve the old test, and reuse it in the new one (the `// Flags:` comments if imported files are not carried over by the test runner, which is good in this case). Also fixed the test failures.
Acknowledged
// Flags: --no-always-sparkplug --expose-gcDrive-by: We never explicitly `gc()` here, so I removed that flag.
Acknowledged
for (let i = 0; i < 10; ++i) {What's the reason for running 10 times before `OptimizeFunctionOnNextCall`? Doesn't the latter enforce that we tier-up and running once unoptimized is sufficient?
There was an issue with lazy feedback allocation IIRC, so it was necessary to run more than once.
Error.prepareStackTrace = (error, frames) => {This function was duplicated, so I decided to pull it into the `testOptimized` helper instead of repeating the comments and assertions.
Acknowledged
// - the line number is always 1
assertEquals(1, frame.getLineNumber());
// - the bytecode offset (1-based)
const wasmFunctionBytecodeOffset = 0x3A;Oh, I forgot all the awesomeness about this like we have one line and the column is not the same as the byte offset as it starts with 1. None of these values are useful for Wasm. 😊
const globalNullArray = builder.addGlobal(wasmRefNullType(array), true, false);I changed the trapping Wasm function to `array.len` on a null array, since that's also supported by the new inlining, whereas `struct.new` was not.
This doesn't work though, the old inlining doesn't support `global.get`, you'd need to pass it as an argument and do `any.convert_extern` to do that which I think the new inlining doesn't support, yet?
Related details
- Daniel Lehmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann added 1 comment![]( "Open in Gerrit")
const globalNullArray = builder.addGlobal(wasmRefNullType(array), true, false);Matthias LiedtkeI changed the trapping Wasm function to `array.len` on a null array, since that's also supported by the new inlining, whereas `struct.new` was not.
This doesn't work though, the old inlining doesn't support `global.get`, you'd need to pass it as an argument and do `any.convert_extern` to do that which I think the new inlining doesn't support, yet?
Uhoh, indeed. The only reference we can pass in is `externref`, but then I cannot convert that for `array.len`. Yet another reason to support `any.convert_extern` and `ref.cast` in the new inlining... I'll pause this CL until I've done that and leave this unresolved as a reminder to change the Wasm body.
Related details
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |