-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Shared memory support #9507
Shared memory support #9507
Conversation
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "fuzzing", "wasmtime:api", "wasmtime:c-api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
crates/c-api/src/types/memory.rs
Outdated
MemoryType::new64(minimum, maximum) | ||
} else { | ||
MemoryType::new( | ||
Box::new(if shared { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind panicking here for now with unimplemented!()
if both shared
and memory64
are set? (we need to add a constructor for that but haven't gotten around to it yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I take this back, this may want to use MemoryTypeBuilder
to support shared 64-bit memories
Suggestions applied! |
Thanks! I think we'll still want to remove the |
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! With MemoryTypeBuilder
for the implementation I think this will be good to go 👍
crates/fuzzing/src/oracles/dummy.rs
Outdated
/// Construct a dummy shared memory for the given memory type. | ||
pub fn dummy_shared_memory(engine: &Engine, ty: MemoryType) -> Result<SharedMemory> { | ||
SharedMemory::new(engine, ty) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed? (or should it perhaps be integrated elsewhere?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I just removed it
* moving dummy shared memory
Oh, nice, the MemoryTypeBuilder made the code way more readable :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a minor issue with clang-format in CI, but otherwise I think this is good to go 👍
Oh, no problem. I can run clang-format and see a green CI :) |
I think there might be something else with clang-format in CI? If you'd like also feel free to mark this as "ready for review" so it can be approved+merged once CI passes. |
Yeah, might be the version of clang format usedd in the CI (15?) has different default options than the one I use locally. |
I dunno what's going on with clang-format myself, I've attempted to resolve the latest issue with a new commit |
I was trying to add Shared Memory support in the wasmtime-py library, but I found out that C-API support here was not finished, so this is my bold attempt to implement something that works.
As far as a I read, threads on WASI are still in discussion and Shared Memory is part of the proposal. In the other hand there's some basic support for Shared memory already present in the Rust API interface, so I guess it's fine to try gluing the C-API with what is supported today, and make wasmtime-py works with Shared Memory (?).