wasmtime / PR #9507 Shared memory support · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9507 Shared memory support

Wasmtime GitHub notifications bot (Oct 24 2024 at 01:49):

atilag opened PR #9507 from atilag:shared-memory-support to bytecodealliance:main:

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 (?).

Wasmtime GitHub notifications bot (Oct 24 2024 at 01:49):

atilag requested alexcrichton for a review on PR #9507.

Wasmtime GitHub notifications bot (Oct 24 2024 at 01:49):

atilag requested wasmtime-fuzz-reviewers for a review on PR #9507.

Wasmtime GitHub notifications bot (Oct 24 2024 at 01:49):

atilag requested wasmtime-core-reviewers for a review on PR #9507.

Wasmtime GitHub notifications bot (Oct 24 2024 at 01:51):

atilag submitted PR review.

Wasmtime GitHub notifications bot (Oct 24 2024 at 01:51):

atilag created PR review comment:

Ok, this is wrong.

Wasmtime GitHub notifications bot (Oct 24 2024 at 02:05):

atilag updated PR #9507.

Wasmtime GitHub notifications bot (Oct 24 2024 at 03:45):

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>

Wasmtime GitHub notifications bot (Oct 24 2024 at 15:39):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 24 2024 at 15:39):

alexcrichton created PR review comment:

There's a split between the wasm_* APIs and wasmtime_* APIs and we can't change the wasm_* ones so this unfortunately can't change. Perhaps rename the todo!() though to a panic!() with an error message along the lines of "please use the wasmtime_* 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 a wasmtime_* method too.

Wasmtime GitHub notifications bot (Oct 24 2024 at 15:39):

alexcrichton created PR review comment:

This I think we'll want to avoid where we decided to split the runtime representation of Memory and SharedMemory but at the type level they're the same type. In that sense the Memory variant above this should be sufficient.

Wasmtime GitHub notifications bot (Oct 24 2024 at 15:39):

alexcrichton created PR review comment:

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)

Wasmtime GitHub notifications bot (Oct 24 2024 at 15:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 24 2024 at 15:52):

alexcrichton created PR review comment:

Actually I take this back, this may want to use MemoryTypeBuilder to support shared 64-bit memories

Wasmtime GitHub notifications bot (Oct 25 2024 at 00:44):

atilag updated PR #9507.

Wasmtime GitHub notifications bot (Oct 25 2024 at 00:46):

atilag commented 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.

Wasmtime GitHub notifications bot (Oct 25 2024 at 00:47):

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.

Wasmtime GitHub notifications bot (Oct 25 2024 at 15:11):

alexcrichton commented on PR #9507:

Thanks! I think we'll still want to remove the SharedMemory arm of ExternType which will help simplify this a bit more though.

Wasmtime GitHub notifications bot (Oct 25 2024 at 21:19):

atilag updated PR #9507.

Wasmtime GitHub notifications bot (Oct 25 2024 at 21:19):

atilag commented on PR #9507:

Done!

Wasmtime GitHub notifications bot (Oct 26 2024 at 15:40):

alexcrichton submitted PR review:

Looking good! With MemoryTypeBuilder for the implementation I think this will be good to go :+1:

Wasmtime GitHub notifications bot (Oct 26 2024 at 15:40):

alexcrichton created PR review comment:

Is this still needed? (or should it perhaps be integrated elsewhere?)

Wasmtime GitHub notifications bot (Oct 30 2024 at 00:49):

atilag updated PR #9507.

Wasmtime GitHub notifications bot (Oct 30 2024 at 00:50):

atilag created PR review comment:

Nah, I just removed it

Wasmtime GitHub notifications bot (Oct 30 2024 at 00:50):

atilag submitted PR review.

Wasmtime GitHub notifications bot (Oct 30 2024 at 00:51):

atilag commented on PR #9507:

Oh, nice, the MemoryTypeBuilder made the code way more readable :)
Done!

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:00):

alexcrichton submitted PR review:

There's a minor issue with clang-format in CI, but otherwise I think this is good to go :+1:

Wasmtime GitHub notifications bot (Oct 31 2024 at 02:08):

atilag updated PR #9507.

Wasmtime GitHub notifications bot (Oct 31 2024 at 02:08):

atilag commented on PR #9507:

Oh, no problem. I can run clang-format and see a green CI :)

Wasmtime GitHub notifications bot (Oct 31 2024 at 18:45):

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.

Wasmtime GitHub notifications bot (Oct 31 2024 at 23:02):

atilag has marked PR #9507 as ready for review.

Wasmtime GitHub notifications bot (Oct 31 2024 at 23:04):

atilag commented on PR #9507:

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!

Wasmtime GitHub notifications bot (Nov 01 2024 at 01:20):

alexcrichton updated PR #9507.

Wasmtime GitHub notifications bot (Nov 01 2024 at 01:20):

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

Wasmtime GitHub notifications bot (Nov 01 2024 at 01:20):

alexcrichton has enabled auto merge for PR #9507.

Wasmtime GitHub notifications bot (Nov 01 2024 at 01:49):

alexcrichton merged PR #9507.

Wasmtime GitHub notifications bot (Nov 05 2024 at 14:12):

atilag edited PR #9507:

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: Nov 22 2024 at 16:03 UTC