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

Add support for shared memory in Wasm. #702

Merged
merged 17 commits into from
Aug 21, 2019
Merged

Add support for shared memory in Wasm. #702

merged 17 commits into from
Aug 21, 2019

Conversation

nlewycky
Copy link
Contributor

Add support for memory regions with the shared flag. These are part of the threads proposal.

  • Add new --enable-threads flags and pass it through to wasmparser.rs and wabt (for .wat files).
  • Enable --enable-threads when parsing spectests. Enables the shared memory region in simd.wast.
  • Adds atomic.wast, the spec test for the threads proposal. With the LLVM backend, all tests pass.
  • Removes the runtime-core/src/memory/static_ directory since there's no need for it to include two implementations. We use the same implementation of static memory for shared and unshared static memory.
  • Implements the atomic instructions in the LLVM backend. However, atomic.load and atomic.store are provided with no atomicity. The rmw and cmpxchg instructions are sequentially consistent.
  • atomic.notify and atomic.wait are unimplemented.

@nlewycky nlewycky changed the title Add support or shared memory in Wasm. Add support for shared memory in Wasm. Aug 20, 2019
}

pub struct SharedMemoryInternal {
memory: RefCell<Box<StaticMemory>>,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe dumb question: shared memory is always static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we've implemented it, yes, but I can't see any reason we couldn't add SharedDynamic if we wanted to. The challenge would be ensuring that the base pointer of the dynamic memory doesn't move while we're forming and using a pointer into that memory. A naive implementation could hold a mutex lock for every memory access in the wasm program.

An efficient implementation might implement base changes by temporarily mapping the same pages at two addresses in memory so that old pointers continue to work during the move. Then, remove the old mapping such that the pointers fault. You can't use that memory again until you can prove all old pointers are gone (otherwise you break Wasm security model). For the wasm program's loads and stores, either it would need to reload the base pointer if the access faults (if we unmap the memory early), or you keep the temporary mapping around until no such pointers exist but that means the grow operation is blocked until then too.

@@ -289,27 +282,56 @@ impl Clone for UnsharedMemory {
}

pub struct SharedMemory {
#[allow(dead_code)]
desc: MemoryDescriptor,
internal: Arc<SharedMemoryInternal>,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need an internal structure? can we move all the fields from SharedMemoryInternal into the SharedMemory itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is the implementation of Clone for SharedMemory. Mind you, I'm suspicious of the fact that we clone memories to begin with, but I took a look and decided that was a problem for another day. The important part is that the underlying size/grow methods share one lock, which is held by SharedMemoryInternal while the SharedMemory can be cloned to your heart's content.

@@ -4395,6 +4448,2024 @@ impl FunctionCodeGenerator<CodegenError> for LLVMFunctionCodeGenerator {
let res = builder.build_bitcast(res, intrinsics.i128_ty, "");
state.push1(res);
}
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented out code, or they could return CodegenError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@bjfish bjfish left a comment

Choose a reason for hiding this comment

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

It would be nice to add a instances with shared memory test for the Rust API side.

@nlewycky
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Aug 21, 2019
702: Add support for shared memory in Wasm. r=nlewycky a=nlewycky

Add support for memory regions with the shared flag. These are part of the threads proposal.

- Add new `--enable-threads` flags and pass it through to wasmparser.rs and wabt (for .wat files).
- Enable `--enable-threads` when parsing spectests. Enables the shared memory region in `simd.wast`.
- Adds `atomic.wast`, the spec test for the threads proposal. With the LLVM backend, all tests pass.
- Removes the `runtime-core/src/memory/static_` directory since there's no need for it to include two implementations. We use the same implementation of static memory for shared and unshared static memory.
- Implements the atomic instructions in the LLVM backend. However, `atomic.load` and `atomic.store` are provided **with no atomicity.** The rmw and cmpxchg instructions are sequentially consistent.
- `atomic.notify` and `atomic.wait` are unimplemented.

Co-authored-by: Nick Lewycky <[email protected]>
@bors bors bot merged commit 8705fe1 into master Aug 21, 2019
@bors bors bot deleted the feature/shared-memory branch August 21, 2019 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants