atilag opened PR #9507 from atilag:shared-memory-support
to bytecodealliance:main
:
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
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 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 (?).
atilag requested alexcrichton for a review on PR #9507.
atilag requested wasmtime-fuzz-reviewers for a review on PR #9507.
atilag requested wasmtime-core-reviewers for a review on PR #9507.
atilag submitted PR review.
atilag created PR review comment:
Ok, this is wrong.
atilag updated PR #9507.
github-actions[bot] commented on PR #9507:
Subscribe to Label Action
cc @fitzgen
<details>
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:
- fitzgen: fuzzing
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton submitted PR review.
alexcrichton created PR review comment:
There's a split between the
wasm_*
APIs andwasmtime_*
APIs and we can't change thewasm_*
ones so this unfortunately can't change. Perhaps rename thetodo!()
though to apanic!()
with an error message along the lines of "please use thewasmtime_*
method instead"?I should also note that if the
wasmtime_*
method doesn't exist yet and you need this method specifically then it's fine to add awasmtime_*
method too.
alexcrichton created PR review comment:
This I think we'll want to avoid where we decided to split the runtime representation of
Memory
andSharedMemory
but at the type level they're the same type. In that sense theMemory
variant above this should be sufficient.
alexcrichton created PR review comment:
Mind panicking here for now with
unimplemented!()
if bothshared
andmemory64
are set? (we need to add a constructor for that but haven't gotten around to it yet)
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Actually I take this back, this may want to use
MemoryTypeBuilder
to support shared 64-bit memories
atilag updated PR #9507.
Suggestions applied!
Wanted to link this issue with [this other one] from wasmtime-py, as this is a required dependency. We will need to rebuild the_bindings.py
after this is merged.
atilag edited a comment on PR #9507:
Suggestions applied!
Wanted to link this issue with this other one from wasmtime-py, as this is a required dependency. We will need to rebuild the_bindings.py
after this is merged.
alexcrichton commented on PR #9507:
Thanks! I think we'll still want to remove the
SharedMemory
arm ofExternType
which will help simplify this a bit more though.
atilag updated PR #9507.
Done!
alexcrichton submitted PR review:
Looking good! With
MemoryTypeBuilder
for the implementation I think this will be good to go :+1:
alexcrichton created PR review comment:
Is this still needed? (or should it perhaps be integrated elsewhere?)
atilag updated PR #9507.
atilag created PR review comment:
Nah, I just removed it
atilag submitted PR review.
Oh, nice, the MemoryTypeBuilder made the code way more readable :)
Done!
alexcrichton submitted PR review:
There's a minor issue with clang-format in CI, but otherwise I think this is good to go :+1:
atilag updated PR #9507.
Oh, no problem. I can run clang-format and see a green CI :)
alexcrichton commented on PR #9507:
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.
atilag has marked PR #9507 as ready for review.
Yeah, might be the version of clang format usedd in the CI (15?) has different default options than the one I use locally.
Anyway, ready!
Thanks for taking a look Alex!
alexcrichton updated PR #9507.
alexcrichton commented on PR #9507:
I dunno what's going on with clang-format myself, I've attempted to resolve the latest issue with a new commit
alexcrichton has enabled auto merge for PR #9507.
alexcrichton merged PR #9507.
atilag edited PR #9507:
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
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 (?).
Last updated: Dec 23 2024 at 12:05 UTC