search

rustwasm / wasm-bindgen

Facilitating high-level interactions between Wasm modules and JavaScript
https://rustwasm.github.io/docs/wasm-bindgen/
Apache License 2.0
7.84k stars 1.08k forks source link

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

Closed RunDevelopment closed 1 month ago

RunDevelopment commented 1 month ago

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.

RunDevelopment commented 1 month ago

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!

RunDevelopment commented 1 month ago

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.

daxpedda commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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!