bytecodealliance / wasm-tools

CLI and Rust libraries for low-level manipulation of WebAssembly modules
Apache License 2.0
1.36k stars 246 forks source link

`wasmparser`: Create abstraction for local initialization (simplify future optimizations) #1870

Closed Robbepop closed 1 month ago

Robbepop commented 1 month ago

This implements the first_non_default index optimization in this comment.

Note that the issue also suggests using a bitmap instead of a Vec<bool>. This is not implemented in this PR as I was unsure if we should implement our own bitvec or use one from crates.io. Furthermore it seems that they are using a bitvec with 32-bit words and 16 words inline storage to avoid heap allocations and indirections for up to 512 locals, thus complicating the matter.

Benchmarks

Minor improvements, not sure if within noise.

image

Robbepop commented 1 month ago

How best to handle the hint::likely unused warnings? They are only used when the validator feature is used, thus we see the warning when validator is disabled. Should we put it under the validator module, put cfgs everywhere, or silence the warnings via #[allow(dead_code)] since it is not actually a validator specific utility.

alexcrichton commented 1 month ago

Thanks for this! Does the likely function help performance enough to keep it? If so I think it's fine to put it in the validator module for now.

Robbepop commented 1 month ago

Thanks for this! Does the likely function help performance enough to keep it? If so I think it's fine to put it in the validator module for now.

When i benchmarked it locally, unfortunately it did.

However, you are welcome to run benchmarks on your machine to test if this is was just a local artifact. I only have a Macbook M2 machine for local benchmarks.

alexcrichton commented 1 month ago

Oh sure yeah that's no problem, just double-checking. Since it's only used in the validator I think it's ok to move it under there to avoid dead code warnings.

Robbepop commented 1 month ago

Oh sure yeah that's no problem, just double-checking. Since it's only used in the validator I think it's ok to move it under there to avoid dead code warnings.

Okay I will fix this later. Note though that the perf gains are modest since most validator regressions are in the type section validation as presented here: https://github.com/bytecodealliance/wasm-tools/issues/1701#issuecomment-2423804444

Robbepop commented 1 month ago

@alexcrichton I moved hint.rs into the validator sub-module.

However, I conducted another benchmark run and mostly saw no improvements anymore compared to main. Still, I think this PR is decent since it will simplify future optimization attempts due to the introduction of the LocalInits abstraction. For example, we could test next to swap out the Vec<bool> with a bit-vec with only internal changes within LocalInits.

Robbepop commented 1 month ago

@alexcrichton I just removed the hint submodule altogether since I no longer see any wins with it.