-
Notifications
You must be signed in to change notification settings - Fork 792
Add memory helpers for using the C-API (from Wasm) #2476
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about what this gets you. If you're already in wasm, can't you just use normal loads and stores? Why do you need functions wrapping the loads and stores?
Was totally expecting, these helpers are somewhat odd :)
If Binaryen is an import to another module, then both modules have their own memory. With the helpers, one can simply do WebAssembly.instantiate(..., { binaryen: binaryen }); without the need to wrap To give an example, what we currently do is to mix in 8-bit store and load functions so we are able to initialize and read strings. With the helpers, we can get rid of the round-trip through JS entirely. |
I see, I wasn't thinking about having separate memories. My first instinct would be to suggest using the same memory for both Binaryen and the module importing it, but of course you then need to buy in to the C++ runtime managing the entire memory. I guess the proper solution here would be to import Binaryen's memory as a second memory in the client module, but of course that would require having multiple memories. I guess I'm fine adding these helpers as long as we remove them as soon as multiple memories becomes usable. Can you add a comment saying that they will be removed in favor of multiple memories in the future? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kripken Does this LGTY?
I don't understand the use case yet. How does the other module have code written in it to use these new exports? Is that handwritten wasm? What has linked and loaded the modules together here - is it not JS? (if JS, then why not use that to handle this as well?) |
In AssemblyScript we can "declare" the Binaryen import by describing its exports, like // binaryen.ts
export declare function _malloc(size: usize): usize;
export declare function _i32_load(ptr: usize): i32; // new
type BinaryenModuleRef = usize;
export declare function _BinaryenModuleCreate(): BinaryenModuleRef;
... but we can't declare an access to Binaryen's memory yet, for example when we are allocating a string for Binaryen to consume or reading a string Binaryen gives us.
Yeah, it is JS. What I'm trying to make possible is: WebAssembly.instantiateStreaming(fetch("asc.wasm"), { binaryen }); instead of binaryen._i32_load8_u = function(ptr) {
return return binaryen.HEAPU8[ptr];
};
WebAssembly.instantiateStreaming(fetch("asc.wasm"), { binaryen });
Idea is that we can "link" a Wasm import to a Wasm export directly without any (custom) JS sitting in between. Not sure if this has any actual benefits in Wasm engines, yet it would allow someone (i.e. currently us) to import Does that make sense? :) (Certainly this is more a "nice to have" at this point, since binaryen.js is still JS and the AssemblyScript compiler is still JS as well - but it's cool). |
Hmm, you do already have custom JS there, right? Is there a downside to including those binaryen._i32_load8_u = function(ptr) {
return return binaryen.HEAPU8[ptr];
}; imports in that code? That feels like the natural place to me to do such a thing. I feel I'm missing the advantage of doing it the way this PR proposes? |
Yes, but it'd become possible to get rid of our custom JS with the helpers. It's not ultimately necessary, of course, so I do understand the hesitation. Just felt like the last missing building block to make the exposed API, as we use it (C-like), "feature complete". Like, there is |
I see, thanks. Yeah, I can understand that is useful there. I don't have a strong objection here, so this sounds fine then. Side note, with Interface Types we can also get rid of this, hopefully, as it would give you the power to read strings etc. across the boundary. But that's not close yet. |
We already have exports for
_malloc
and_free
in the Emscripten build, but there is no way yet to initialize the data without resorting to JS. Hence this PR adds a few additional memory helpers to the Emscripten build so it becomes possible to manipulate Binaryen memory without the need for extra glue code, for example when Binaryen is a WebAssembly import, and one is allocating strings to be used by / reading strings returned by Binaryen.I expect this to be a bit controversial because the use case is relatively specific, but it makes sense for us because we are consuming the C-API directly (from JS and eventually Wasm) and don't rely on
binaryen.js-post.js
.