-
Notifications
You must be signed in to change notification settings - Fork 691
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
Thoughts on making wasm2c thread-safe #1805
Comments
I believe that the "per-instance" part of #1721 should cover this. We expect to see separate PRs for those, including one for instancing. cc @shravanrn |
Oh, actually I see you linked to #1721 already but with a About the overhead, I am not very worried. I'd expect an LTO build to be able to recover any losses. I could be wrong though... |
Hi, I am hoping to make more progress landing that commit early February and will keep you posted. Regarding your question re multiple memory accesses. I agree with alon that this isn't a huge issue as this extra load to the the "data" field wouldn't happen every time --- at most a few times per function. So net performance difference wouldn't be that big. More concretely, this is a standard problem with sandboxing. (You may already be familiar with this, so I apologize if this is a repeat). In short, the only way to avoid the multiple memory safe accesses for an AOT multiple instance sandboxing compiler is to reserve a register which exclusively holds the value for data (the register cannot be used for any other computation). The expectation is that pinned registers would buy a few percent of speedup in code with a lot of memory accesses(I think cranelift/wasmtime has some benchmarks in their compiler that show this), but this change would slow down CPU bottlenecked code due to the added register pressure. So depending on computation, this may or may not be something you'd want. More to the point for wasm2c, there is no C compliant way to pin registers (the register keyword in C does not do the right thing for this). You'd have to mess with the core of the compiler to get the right behavior for pinned registers. Given these factors, my own take is that the current wasm2c approach is the right tradeoff. |
It sounds like we're on the same page. PR submitted. |
Hello all,
I would like to start a conversation about the best way to make wasm2c thread-safe so that the same module can be instantiated multiple times with different memory contexts, and functions from different instances can be executed in parallel. Our current plan was to move global memory variables into a structure and then pass a pointer to that structure as a parameter to every function call (Similar changes are also proposed by #1721). Before we draft a PR, I would like to see what are people’s thoughts about this. More details are below.
Since the changes we make introduce a new layer of abstraction, we were concerned about the potential increase of memory access latency. Therefore, we investigated the assembly code generated by the output of status quo wasm2c and a toy model of the expected output of our new version of wasm2c in terms of memory access. The assembly code is compiled with
clang -O2
. Both versions require 2 memory access for a memory store.Status quo wasm2c
Input wat file:
Assembly code compiled from wasm2c output:
There are two memory access for the actual
i32_store
,401259: 48 8b 0d 40 2e 00 00 mov 0x2e40(%rip),%rcx
for storing the address ofw2c_M0->data
to a register and401260: 89 34 01 mov %esi,(%rcx,%rax,1)
for the actual storing.Toy model of the expected output after our changes
program.cc
main.cc
and the compiled assembly code:
There are also two memory access,
0: 48 8b 47 10 mov 0x10(%rdi),%rax
for gettingmem->memorytwo.data
and6: 40 88 34 08 mov %sil,(%rax,%rcx,1)
for the actual store.Both the status quo and our version of wasm2c has the same memory access overhead on the assembly code level. It would be nice if someone could share their thoughts on the memory access pattern of wasm2c and also the changes we are planning to make.
The text was updated successfully, but these errors were encountered: