- Notifications
You must be signed in to change notification settings - Fork396
Wasm: Make itables immutable struct#5038
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good overall. Only minor comments.
| /** Generate common taible global for all array classes | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Typo + formatting:
| /** Generate common taible global for all array classes | |
| */ | |
| /** Generate common itable global for all array classes.*/ |
linker/shared/src/main/scala/org/scalajs/linker/backend/wasmemitter/VarGen.scalaShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| if interfaceInfo.isInterface | ||
| } { | ||
| val init = interfaceInfo.tableEntries.map { method => | ||
| ctx.refFuncWithDeclaration(resolvedMethodInfos(method).tableEntryID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since this is now in aglobal section, we don't need separate declarations anymore:
| ctx.refFuncWithDeclaration(resolvedMethodInfos(method).tableEntryID) | |
| wa.RefFunc(resolvedMethodInfos(method).tableEntryID) |
| val global = wamod.Global( | ||
| classITableGlobalID, | ||
| makeDebugName(ns.ITable, classInfoForResolving.name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is going to be confusing for the itables array of array classes. Previously we had
OriginalName(genGlobalID.arrayClassITable.toString())for that one. Pass it as an additional parameter togenGlobalClassItable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh yeah thanks, I added anoriginalName parameter togenGlobalClassItable
Previously, we used mutable array-based interface tables (itables) without a specific reason.However, accessing mutable arrays can lead to missed optimization opportunities.For instance, loop-invariant code motion (LICM) won't be able to move certain parts of interface dispatching operations out of loops:When we invoke a method using interface table dispatching, it requires retrieving an itable from the class's itable array, which is considered non-pure.In fact, reading from an array is marked as unsafe to move in wasm-opt's LICM implementation:https://github.com/WebAssembly/binaryen/blob/34ad6a7598e662e9ff357987f2c81fde1e05c522/src/ir/effects.h#L207-L210https://github.com/WebAssembly/binaryen/blob/34ad6a7598e662e9ff357987f2c81fde1e05c522/src/passes/LoopInvariantCodeMotion.cpp#L125-L128(Note: We may not run wasm-opt's LICM in favor of the VM's LICM, as discussed inWebAssembly/binaryen#6924)This commit changes itables to be immutable structs, making itable retrieval a pure operation.Here's a comparison of benchmark results between commit4c9494e and this change:| | array itables | struct itables || ----------- | ------------- | -------------- || sha512 | 12128.73213 | 12107.94142 || queens | 3.632068223 | 3.655815179 || list | 84.61223471 | 65.69730564 || richards | 92.12833022 | 91.19028905 || cd | 46267.10318 | 46574.81215 || gcbench | 116540.568 | 112594.324 || tracerFloat | 2569.675034 | 2551.227714 || tracer | 1725.591249 | 1697.907466 || sudoku | 14863.33909 | 12066.23882 || nbody | 366388.3292 | 199625.971 || permute | 1006.215249 | 983.9858283 || deltaBlue | 2205.002528 | 2320.038418 || kmeans | 1752457.829 | 1754425.973 || brainfuck | 36983.97583 | 36093.7837 || json | 1205.92764 | 1191.768986 || bounce | 21.14001572 | 20.8851082 |While most benchmark results remain same, some tests such as `list`, `sudoku`, and `nbody` show improved performance compared to the array-based itables (may be thanks to the VM's optimization capabilities).**immutable array?**In this change, we implement itables using immutable structsHowever, we can use immutable arrays as an alternative.We implemented and benchmarked itables using immutable arrays, and the results show nearly same performance as the immutable struct-based implementation.WebAssembly/gc#250v8/v8@6e36e3eLet's proceed with the immutable struct-based implementation for now because- While V8 seems to optimize immutable array-related operations, other VMs may not offer similar optimizations (?)- Currently, wasm-opt doesn't take into account the mutability of arrays in its optimization process.
4fbda49 to614e228Compare76fef2e intoscala-js:mainUh oh!
There was an error while loading.Please reload this page.
Previously, we used mutable array-based interface tables (itables) without a specific reason. However, accessing mutable arrays can lead to missed optimization opportunities. For instance, loop-invariant code motion (LICM) won't be able to move certain parts of interface dispatching operations out of loops: When we invoke a method using interface table dispatching, it requires retrieving an itable from the class's itable array, which is considered non-pure.
In fact, reading from an array is marked as unsafe to move in wasm-opt's LICM implementation:
https://github.com/WebAssembly/binaryen/blob/34ad6a7598e662e9ff357987f2c81fde1e05c522/src/ir/effects.h#L207-L210https://github.com/WebAssembly/binaryen/blob/34ad6a7598e662e9ff357987f2c81fde1e05c522/src/passes/LoopInvariantCodeMotion.cpp#L125-L128
(Note: We may not run wasm-opt's LICM in favor of the VM's LICM, as discussed inWebAssembly/binaryen#6924)
This commit changes itables to be immutable structs, making itable retrieval a pure operation. Here's a comparison of benchmark results between commit4c9494e and this change:
While most benchmark results remain same, some tests such as
list,sudoku, andnbodyshow improved performance compared to the array-based itables (may be thanks to the VM's optimization capabilities).immutable array?
In this change, we implement itables using immutable structs However, we can use immutable arrays as an alternative. We implemented and benchmarked itables using immutable arrays, and the results show nearly same performance as the immutable struct-based implementation.
WebAssembly/gc#250
v8/v8@6e36e3e
Let's proceed with the immutable struct-based implementation for now because