-
Notifications
You must be signed in to change notification settings - Fork 58
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
Rust Wasm ABI is unstable and will be changed eventually #661
Comments
Do we have an idea of how Knowing how "crossing the WASM ABI" hasn't seemed to have been an area where enough people really have put enough effort, there might be room to influence the outcome upstream, especially if we can share performance numbers about how much faster certain real-world ICU4X operations might be in the unstable Rust Wasm ABI versus the "spec" Wasm ABI. |
The ABI that rustc currently uses is effectively unsound due to rust-lang/rust#115666 -- or at least, it is highly unclear whether it satisfies our ABI compatibility rules. So I don't think we should keep using it under any circumstances. If the wasm ABI can be improved, that should be done as a wasm-wide change, not by rustc unilaterally using a different |
It basically means that Diplomat slices and ICU4X options bags need to be allocated on the Rust heap before being passed to Rust from JS. Instead of: wasm.foo(field1, field2); we will have to do let buffer = diplomat_alloc(enough space for struct);
ptrWrite(buffer, 0, field1);
ptrWrite(buffer, offset, field2);
wasm.foo(buffer.ptr);
``
There are wasm-wide discussions on doing better here, as Ralf [linked](https://github.com/WebAssembly/tool-conventions/issues/88). We can participate, I'm not sure if we have anything further to bring to the table than what is already being said there.
They seem to be moving towards "pass small structs as fields, pass large structs by pointer", which will kind of be similar to what Rust already does, but with more complexity because of the pointer (whereas currently it's "pass small structs as fields, pass large structs as fields _with padding_"). |
I care slightly less about options bags, but things like DiplomatWriteable and DiplomatSlice are in the critical path for hot functions like format terminals, so I would hope that those don't need an alloc. I guess a workaround would be to backtrack a bit and inline those structs back to separate parameters in the C functions. Maybe only when the target is WASM. |
You can manually split slices into two arguments when generating the |
Generally not a great idea since Diplomat's design is that at the C level it's always the same. And in particular this would mean complicating the macro again (and in a way that's conditional on wasm!), which isn't great.
Yes, and we used to do this, but now we consistently use a DiplomatSlice repr(C) struct, which greatly simplifies the code. So yeah, in theory something we can do, in practice would rather not choose this route.
Note that allocs aren't that bad here. Allocs are bad in native code, but JS hits the allocator all the time, and while the Rust-side allocator in wasm is not the JS allocator, hitting it is not that expensive compared to other things in JS. I don't think this slows down format functions in a way that's going to be super noticeable. Measurable, probably, noticeable, probably not, it's going to be a same-order-of-magnitude change. |
Either way, the proposed ABI of "small structs are passed directly, large structs indirectly" woudl solve this for us. |
I think you can avoid the allocator by storing everything on the stack instead. You can decrement the stack pointer global before the call, store the arguments at the new stack pointer location, perform the call and restore the stack pointer. This will need the stack pointer to be exported by the wasm module though, or alternatively you need to export a function which adjusts the stack pointer as requested while not touching the stack at all. The latter probably requires hand written wasm. |
Not sure if anyone has given any more thought to this, but I've been doing some testing in the repo with WIP notes are here: https://docs.google.com/document/d/1F0hvY9J8IsRz7wS0allO_E9pu75zKXudDcNWjumx-HA/edit?usp=sharing For what I've messed with so far, it seems like structs are handled almost the same as we handle them now (padding logic included), with the exception that we are passing structs (and slices!) as allocated pointers, rather than as tuples. There does not seem to be a distinction based on the size of the struct. Not sure if the ABI is going to change, but I'd be happy to open a PR based on making Diplomat compatible with what exists already. It could be as simple as a tool flag (or detecting the |
I think it should be a tool config (though perhaps we should get our holistic config first: #772). I would love to see if this can be done with minimal code complexity, otherwise it might be better to just unconditionally move to the new model. |
I believe minimal complexity is totally doable, so long as no one minds say, a struct having to be allocated in memory every time we want to pass it in as a parameter. Another solution I can think of is to keep the struct allocated in memory and update it every time a field is changed in JS. This could still work without a major re-write, as most of the changes for both options I've presented would be in the template files (and don't require major structural re-works as a result) |
yeah that's inevitable in this model. It's fine. We could optimize this by having some sort of global JS arena so we can reuse buffers in JS. The nice thing is that there's no complicated GC needed: struct allocs are always freed after a call.
This is the older architecture in the JS backend, I don't like it. Diplomat's design strongly prefers conversion over the boundary |
Single field structs should be passed the same as their only field: https://github.com/WebAssembly/tool-conventions/blob/main/BasicCABI.md#function-arguments-and-return-values |
Yup! That's the case for the current ABI as well; I forgot to note that down because our tests to catch it were already passing. I'll make sure to update the notes though, thanks! |
But if the conversion approach sounds good, I'd be happy to get started on that! My plan of action:
|
IMO go for it wrt implementing configs. Priority is a shared config system that can be specified in files, and then we can work on ways to override it. |
Previously: #657 (talking about the specific issue we found)
As explained in rust-lang/rust#129486, the Rust Wasm ABI is currently not following tool conventions and is unstable. rust-lang/rust#117919 details the plan to move forward from here, with a
-Zwasm-c-abi=spec
flag that will become default over time.This ABI is one where all non-scalar structs are passed as pointers, which means that JS will have to preallocate all structs (except those that wrap a single scalar) and pass them in as pointers. More expensive, but fine.
On the plus side, we can get rid of a lot of the finicky code from #660.
We don't have to do this now, but we should before Rust changes defaults. When we do we should document it. We can potentially support both forms if people really ask for it, but it could get pretty gnarly.
The text was updated successfully, but these errors were encountered: