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

Optimized ABI for Option<u32> and other 32-bit primitives #4183

Merged
merged 15 commits into from
Oct 16, 2024

Conversation

RunDevelopment
Copy link
Contributor

This is a PoC to optimize the ABI of Option<u32> (another other 32-bit primitives) by using f64.

The basic idea is that we can avoid going through "main memory" by using f64. f64 is large enough to represent all possible values of 32-bit primitives and a hole for None. In this case, I used 232+1 for the hole, because it cannot be represented by any of the 32-bit primitives (even f32 can't represent it exactly). I then use the same sentinel trick wasm bindgen already uses for Option<u16> and other primitives with <32 bit. So please think of this PR as an extension of that trick.


The current state of this PR is just a PoC. It works, but I haven't tested it a lot. I would like to know whether this is something the wasm bindgen people are interested in before polishing it some more.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does make sense to me, I'm just asking myself why this wasn't considered before.

I'm definitely interested in reviewing this, but I will have to do some digging and see if there is any historical context around this.

@daxpedda daxpedda added the needs review Needs a review by a maintainer. label Oct 12, 2024
@RunDevelopment
Copy link
Contributor Author

I'm just asking myself why this wasn't considered before.

Maybe they didn't like the weird ABI?

Otherwise, float to integer conversion could have scared them away, because they can be tricky. In particular, I haven't looked up whether the code I wrote for going f64 (ABI) -> i32/u32/f32 behaves the same way as what the WebAssembly API normally does for us. I will have to look up the spec for that some time.

I will have to do some digging and see if there is any historical context around this.

Thank you for doing this!

@daxpedda daxpedda added waiting for author Waiting for author to respond and removed needs review Needs a review by a maintainer. labels Oct 14, 2024
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was briefly mentioned by alexcrichton in #630 (comment), not sure why it wasn't picked up.

AFAIU this should work just fine, so lets go ahead with this!

src/convert/impls.rs Outdated Show resolved Hide resolved
crates/cli-support/src/js/binding.rs Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Contributor Author

Okay, I think this PR is pretty much ready.

Aside from the 32-bit primitives, I also had to enable this optimization for Option<*mut T> and Option<*const T>. Something in wasm bindgen assumes that Option<Ptr> has the same ABI as Option<u32>, so I had to make that assumption true again to make it work. Honestly, not a big ask since it's a good idea anyway.
For the record: I did not go through the code to base to find which part makes this assumption. I would want this assumption to be true, so I don't see the need to change anything.

For debugging purposes, I also added a new debug_name parameter that is passed into JsBuilder. This allows for better error messages that make it waaay easier to figure out which function causes JS code gen to fail. Before this change, the asserts in JsBuilder just crashed the program with no indication of which function or even which test went wrong. This change will hopefully make it easier to make ABI and JS code gen changes.

@RunDevelopment RunDevelopment marked this pull request as ready for review October 15, 2024 20:07
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits, but LGTM.
The debugging improvement is amazing as well!

crates/cli-support/src/js/binding.rs Outdated Show resolved Hide resolved
tests/wasm/optional_primitives.js Outdated Show resolved Hide resolved
tests/wasm/optional_primitives.js Outdated Show resolved Hide resolved
src/convert/impls.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
crates/cli-support/src/js/binding.rs Show resolved Hide resolved
@daxpedda
Copy link
Collaborator

Aside from the 32-bit primitives, I also had to enable this optimization for Option<*mut T> and Option<*const T>. Something in wasm bindgen assumes that Option<Ptr> has the same ABI as Option<u32>, so I had to make that assumption true again to make it work. Honestly, not a big ask since it's a good idea anyway.
For the record: I did not go through the code to base to find which part makes this assumption. I would want this assumption to be true, so I don't see the need to change anything.

I didn't get what you are trying to say here.
Make what assumption true again?

In any case, it all looks correct to me.

@RunDevelopment
Copy link
Contributor Author

Make what assumption true again?

Something (I didn't look into it) in the code base assumes that Option<*T> and Option<u32> have the same ABI. I initially didn't change the ABI of Option<*T>, but this caused tests to fail (see CI logs). So I made those ABIs match again to fix the bug caught by the tests.

@daxpedda
Copy link
Collaborator

Make what assumption true again?

Something (I didn't look into it) in the code base assumes that Option<*T> and Option<u32> have the same ABI. I initially didn't change the ABI of Option<*T>, but this caused tests to fail (see CI logs). So I made those ABIs match again to fix the bug caught by the tests.

Ah, I see!

@daxpedda daxpedda merged commit 7c7cd51 into rustwasm:main Oct 16, 2024
41 checks passed
@RunDevelopment RunDevelopment deleted the f64-abi-for-32-bit-types branch October 16, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants