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

feat: Add v8::CompiledWasmModule #776

Merged
merged 3 commits into from
Sep 17, 2021

Conversation

andreubotella
Copy link
Contributor

v8::CompiledWasmModule is a representation of a compiled WebAssembly module, which can be shared by multiple v8::WasmModuleObjects.

Closes #759.

@andreubotella
Copy link
Contributor Author

In order for v8::CompiledWasmModule to be useful for deno, it must impl Send, and a naive implementation (using shared code with SharedArrayBufferStore) would also need it to impl Sync. Not being too fluent in C++, I'm not sure how sound that would be.

Andreu Botella added 2 commits September 13, 2021 13:17
`v8::CompiledWasmModule` is a representation of a compiled WebAssembly
module, which can be shared by multiple `v8::WasmModuleObject`s.

Closes denoland#759.
src/wasm.rs Show resolved Hide resolved
let foo_bs = foo_ab.get_backing_store();
let foo_section = unsafe {
std::slice::from_raw_parts(foo_bs.data() as *mut u8, foo_bs.byte_length())
};
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just do the conversation to string in JavaScript?

let ab = WebAssembly.Module.customSections(module, 'foo')[0];
let ui8 = new Uint8Array(ab);
new TextDecoder().decode(ui8)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TextDecoder is a web API, not a v8 built-in.


// TODO(andreubotella): Safety???
unsafe impl Send for CompiledWasmModule {}
unsafe impl Sync for CompiledWasmModule {}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this either. If you don't need it, can we leave them out for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're gonna be needed for denoland/deno#11823 though.

src/wasm.rs Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - nice implementation - these are useful bindings to have!

@ry ry merged commit babe41a into denoland:main Sep 17, 2021
@andreubotella andreubotella deleted the compiled-wasm-module branch September 17, 2021 16:45
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.

Add bindings to serialise and deserialise WASM module objects
2 participants