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

Support WebAssembly.instantiateStreaming #21130

Closed
Leko opened this issue Jun 5, 2018 · 17 comments · Fixed by #42701
Closed

Support WebAssembly.instantiateStreaming #21130

Leko opened this issue Jun 5, 2018 · 17 comments · Fixed by #42701
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js. stalled Issues and PRs that are stalled. v8 engine Issues and PRs related to the V8 dependency.

Comments

@Leko
Copy link
Contributor

Leko commented Jun 5, 2018

I think it's better support WebAssembly.instantiateStreaming.
It makes more easy to use WebAssembly and we can get more compatibility for Web.

It already implemented in Google Chrome, Firefox and some browsers.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/instantiateStreaming

instantiateStreaming already exists in deps/v8/src/wasm/wasm-js.cc but cannot cover this branch.

if (isolate->wasm_compile_streaming_callback() != nullptr) {
InstallFunc(isolate, webassembly, "compileStreaming",
WebAssemblyCompileStreaming, 1);
InstallFunc(isolate, webassembly, "instantiateStreaming",
WebAssemblyInstantiateStreaming, 1);
}

To cover this branch, we must call SetWasmCompileStreamingCallback.

void SetWasmCompileStreamingCallback(ApiImplementationCallback callback);

node/deps/v8/src/api.cc

Lines 8876 to 8877 in de73272

CALLBACK_SETTER(WasmCompileStreamingCallback, ApiImplementationCallback,
wasm_compile_streaming_callback)

In chromium, implemented here:
https://github.com/chromium/chromium/blob/51459d663d841c6430747aec97be9f7e7a7ca41f/third_party/blink/renderer/bindings/core/v8/v8_wasm_response_extensions.cc#L194-L222
And use it here:
https://github.com/chromium/chromium/blob/51459d663d841c6430747aec97be9f7e7a7ca41f/third_party/blink/renderer/bindings/core/v8/v8_wasm_response_extensions.cc#L228

Why we must inject the actual implementation,
It's said that this is for layering reasons.
WebAssembly/design#1085

Discussion

  1. How about it?
  2. How to make instantiateStreaming compatible with design.
    • ex. instantiateStreaming(fs.promises.readFile('./some.wasm'), importObject)
      • It's not compatible with design.
      • readFile returns Buffer, not ArrayBuffer.
@Leko Leko added discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js. labels Jun 5, 2018
@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Jun 5, 2018
@devsnek
Copy link
Member

devsnek commented Jun 5, 2018

The design of instantiateStreaming currently requires Response to perform proper checks for stuff like mimes and whatnot. While node could implement instantiateStreaming possibly taking a node Stream object i don't think its worth splintering the wasm api between browsers and node. For some reasons i won't go into here node might gain a Response object anyway (and we're already working on mimes over in #21128) so once we get that stuff in this could be done.

@AyushG3112
Copy link
Contributor

While I understand that implementing WebAssembly for isomorphism might be good, but, for native modules, we already have Native and N-API addons. This would just be throwing another one into the mix.

@devsnek
Copy link
Member

devsnek commented Jun 5, 2018

@AyushG3112 we already support wasm, this would just be adding another method of loading it.

@AyushG3112
Copy link
Contributor

@devsnek ah, I did not know that. Nevermind my comment then, and thanks!

@bnoordhuis
Copy link
Member

What is the use case besides feature parity with browsers? WebAssembly.instantiateStreaming() seems of limited usefulness in Node.js. Likewise WebAssembly.compileStreaming().

ex. instantiateStreaming(fs.promises.readFile('./some.wasm'), importObject)

That would be fairly straightforward to implement but I share @devsnek's sentiment that we shouldn't diverge from browsers lightly.

For some reasons i won't go into here node might gain a Response object

Pray tell!

@Leko
Copy link
Contributor Author

Leko commented Jun 5, 2018

@devsnek @bnoordhuis I totally agree.
I think it should be implemented the same API as browser.

@bnoordhuis
Copy link
Member

Okay, but can you speak to your use case? As I said, I don't really see when or why you'd use it with Node.js.

@Leko
Copy link
Contributor Author

Leko commented Jun 5, 2018

@bnoordhuis My use case is image processor what work on browser and Node.js implemented by WebAssembly.
And small wrapper for loading it.

BTW, I think we decide to implement Fetch API or not prior to start this discussion.
It already discussing in this issue: #19393 (comment)

@bnoordhuis
Copy link
Member

image processor what work on browser and Node.js implemented by WebAssembly

I can see why you'd load it in streaming fashion in a browser but why in Node.js? If it's an on-disk file, just load it in one go.

@Leko
Copy link
Contributor Author

Leko commented Jun 7, 2018

@bnoordhuis
Personally, The most important reason is portability.
I want to use instantiateStreaming because Google recommended way of loading WebAssembly to use WebAssembly.*Streaming.
But cannot use instantiateStreaming if planned to run both Node.js and browser.

@xtuc
Copy link

xtuc commented Jun 26, 2018

What if the WebAssembly loading and instantiation is done is a separate worker and posted to the main thread?

Also FYI https://github.com/wasm-tool/node-loader. I know that @bmeck is working a multi-threaded loading which would fit well to this use case.

danbev added a commit to danbev/node that referenced this issue Aug 15, 2018
This commit adds InstantiateStreaming support to allow for some
compatability with Web browser environments.

Refs: nodejs#21130
@Trott
Copy link
Member

Trott commented Oct 26, 2018

Is this something Node.js is likely to implement? If not, should we close this?

@Trott
Copy link
Member

Trott commented Nov 10, 2018

I'm going to mark this as stalled and close it. Feel free to comment or re-open if you believe that's the wrong thing to do here.

@Trott Trott closed this as completed Nov 10, 2018
@Trott Trott added the stalled Issues and PRs that are stalled. label Nov 10, 2018
@tigercosmos
Copy link

It's sad that Node doesn't support this method. 😐

@tniessen
Copy link
Member

tniessen commented Sep 5, 2020

It's sad that Node doesn't support this method. 😐

I am sure we would consider a PR if you were to implement it.

@devsnek
Copy link
Member

devsnek commented Sep 5, 2020

good start here: https://github.com/devsnek/node/tree/feature/wasm-streaming

@tniessen tniessen reopened this Apr 12, 2022
tniessen added a commit to tniessen/node that referenced this issue Apr 12, 2022
@devsnek
Copy link
Member

devsnek commented Apr 12, 2022

👀

tniessen added a commit to tniessen/node that referenced this issue Apr 12, 2022
tniessen added a commit to tniessen/node that referenced this issue Apr 12, 2022
tniessen added a commit to tniessen/node that referenced this issue Apr 12, 2022
tniessen added a commit to tniessen/node that referenced this issue Apr 13, 2022
tniessen added a commit to tniessen/node that referenced this issue Apr 17, 2022
RaisinTen pushed a commit to RaisinTen/node that referenced this issue Apr 17, 2022
tniessen added a commit to tniessen/node that referenced this issue Apr 22, 2022
tniessen added a commit to tniessen/node that referenced this issue Apr 22, 2022
tniessen added a commit to tniessen/node that referenced this issue Apr 22, 2022
tniessen added a commit to tniessen/node that referenced this issue Apr 22, 2022
nodejs-github-bot pushed a commit that referenced this issue Apr 23, 2022
Refs: #41749
Fixes: #21130

PR-URL: #42701
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
Refs: nodejs#41749
Fixes: nodejs#21130

PR-URL: nodejs#42701
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Apr 28, 2022
Refs: #41749
Fixes: #21130

PR-URL: #42701
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js. stalled Issues and PRs that are stalled. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants