Skip to content
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

wiggle: add initial support for shared memory #5225

Merged
merged 4 commits into from
Nov 8, 2022

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Nov 8, 2022

This change is the first in a series of changes to support shared memory in Wiggle. Since Wiggle was written under the assumption of single-threaded guest-side access, this change introduces a shared field to guest memories in order to flag when this assumption will not be the case. This change always sets shared to false; once a few more pieces are in place, shared will be set dynamically when a shared memory is detected, e.g., in a change like #5054.

Using the shared field, we can now decide to load Wiggle values differently under the new assumptions. This change makes the guest T::read and T::write calls into Relaxed atomic loads and stores in order to maintain WebAssembly's expected memory consistency guarantees. We choose Rust's Relaxed here to match the Unordered memory consistency described in the memory model section of the ECMA spec.

Since 128-bit scalar types do not have Atomic* equivalents, we remove their T::read and T::write implementations here. They are unused by any WASI implementations in the project.

This change is the first in a series of changes to support shared memory
in Wiggle. Since Wiggle was written under the assumption of
single-threaded guest-side access, this change introduces a `shared`
field to guest memories in order to flag when this assumption will not
be the case. This change always sets `shared` to `false`; once a few
more pieces are in place, `shared` will be set dynamically when a shared
memory is detected, e.g., in a change like bytecodealliance#5054.

Using the `shared` field, we can now decide to load Wiggle values
differently under the new assumptions. This change  makes the guest
`T::read` and `T::write` calls into `Relaxed` atomic loads and stores in
order to maintain WebAssembly's expected memory consistency guarantees.
We choose Rust's `Relaxed` here to match the `Unordered` memory
consistency described in the [memory model] section of the ECMA spec.

[memory model]: https://tc39.es/ecma262/multipage/memory-model.html#sec-memory-model

Since 128-bit scalar types do not have `Atomic*` equivalents, we remove
their `T::read` and `T::write` implementations here. They are unused by
any WASI implementations in the project.
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One interesting option here is to actually always use atomic loads since Relaxed shouldn't be all that different from non-atomic in this context perf-wise. It might even be a tiny bit faster with the lack of a branch perhaps?

crates/wiggle/src/guest_type.rs Outdated Show resolved Hide resolved
crates/wiggle/src/guest_type.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Nov 8, 2022
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

Subscribe to Label Action

cc @kubkon

This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

  • kubkon: wasi

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Values must be little-endian for WebAssembly's sake; this adds
conversions to and from the host's native endianness. Additionally, this
splits out the macro into `integer_primitives!` and `float_primitives!`
versions.
@abrown abrown marked this pull request as ready for review November 8, 2022 18:01
@abrown abrown requested a review from alexcrichton November 8, 2022 18:31
@abrown
Copy link
Contributor Author

abrown commented Nov 8, 2022

cc: @pchickey

crates/wiggle/src/guest_type.rs Outdated Show resolved Hide resolved
@abrown abrown merged commit f026d95 into bytecodealliance:main Nov 8, 2022
@abrown abrown deleted the shmem-in-wiggle-read-write branch November 8, 2022 21:25
abrown added a commit to abrown/wasmtime that referenced this pull request Nov 14, 2022
`wiggle` looks for an exported `Memory` named `"memory"` to use for its
guest slices. This change allows it to use a `SharedMemory` if this is
the kind of memory used for the export.

It is `unsafe` to use shared memory in Wiggle because of broken Rust
guarantees: previously, Wiggle could hand out slices to WebAssembly
linear memory that could be concurrently modified by some other thread.
With the introduction of Wiggle's new `UnsafeGuestSlice` (bytecodealliance#5225, bytecodealliance#5229,
 bytecodealliance#5264), Wiggle should now correctly communicate its guarantees through
its API.
abrown added a commit to abrown/wasmtime that referenced this pull request Nov 15, 2022
`wiggle` looks for an exported `Memory` named `"memory"` to use for its
guest slices. This change allows it to use a `SharedMemory` if this is
the kind of memory used for the export.

It is `unsafe` to use shared memory in Wiggle because of broken Rust
guarantees: previously, Wiggle could hand out slices to WebAssembly
linear memory that could be concurrently modified by some other thread.
With the introduction of Wiggle's new `UnsafeGuestSlice` (bytecodealliance#5225, bytecodealliance#5229,
 bytecodealliance#5264), Wiggle should now correctly communicate its guarantees through
its API.
alexcrichton pushed a commit that referenced this pull request Nov 15, 2022
`wiggle` looks for an exported `Memory` named `"memory"` to use for its
guest slices. This change allows it to use a `SharedMemory` if this is
the kind of memory used for the export.

It is `unsafe` to use shared memory in Wiggle because of broken Rust
guarantees: previously, Wiggle could hand out slices to WebAssembly
linear memory that could be concurrently modified by some other thread.
With the introduction of Wiggle's new `UnsafeGuestSlice` (#5225, #5229,
 #5264), Wiggle should now correctly communicate its guarantees through
its API.
@abrown abrown added the wasm-proposal:threads Issues related to the WebAssembly threads proposal label Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI wasm-proposal:threads Issues related to the WebAssembly threads proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants