Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Oct 23, 2024

Conversation

Robbepop
Copy link
Contributor

@Robbepop Robbepop commented Oct 17, 2024

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

This will make it simpler to optimize the internals since it won't change the usage sites.
Benchmarks showed that they have a minor effect.
@Robbepop
Copy link
Contributor Author

Robbepop commented Oct 17, 2024

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
Copy link
Member

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
Copy link
Contributor Author

Robbepop commented Oct 22, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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: #1701 (comment)

@Robbepop
Copy link
Contributor Author

Robbepop commented Oct 23, 2024

@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
Copy link
Contributor Author

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

@Robbepop Robbepop changed the title wasmparser: Optimize local initialization tracking wasmparser: Create abstraction for local initialization (simplify future optimizations) Oct 23, 2024
@alexcrichton alexcrichton added this pull request to the merge queue Oct 23, 2024
Merged via the queue into bytecodealliance:main with commit 89ff546 Oct 23, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants