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

lib,src: implement WebAssembly Web API #42701

Merged
merged 1 commit into from Apr 23, 2022

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Apr 12, 2022

Now that we have fetch(), we may as well also have WebAssembly.compileStreaming() and WebAssembly.instantiateStreaming().

Unfortunately, our fetch() implementation cannot be interacted with easily from C++, which is why part of this implementation ended up in JavaScript. The C++ glue code allows controlling the v8::WasmStreaming instance from JavaScript.

This attempts to be a spec-compliant implementation with no node-specific extensions.

Refs: #41749
Fixes: #21130

@tniessen tniessen added notable-change wasm fetch labels Apr 12, 2022
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 12, 2022

Review requested:

@nodejs-github-bot nodejs-github-bot added lib / src needs-ci labels Apr 12, 2022
new WasmStreamingObject(env, args.This());
}

void WasmStreamingObject::Push(
Copy link
Member

@devsnek devsnek Apr 12, 2022

Choose a reason for hiding this comment

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

unless i am mistaken, these methods do not allocate. would you be up for adding fast call wrappers? if not i can maybe add them in a followup pr.

Copy link
Member Author

@tniessen tniessen Apr 12, 2022

Choose a reason for hiding this comment

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

Will try that tomorrow. So far, every time I tried using fast calls, I can into some V8 limitation, but Push might be a good candidate for once.

Copy link
Member Author

@tniessen tniessen Apr 21, 2022

Choose a reason for hiding this comment

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

On second thought, given how web streams are performing so far and how slow everything is, that seems like premature optimization. I might create a follow-up PR to add that or I might push that as a separate commit later, but I'd like to get this landed rather sooner than later and not make it more complicated to review.

@targos targos added the semver-minor label Apr 12, 2022
doc/api/errors.md Show resolved Hide resolved
src/node_wasm_web_api.h Outdated Show resolved Hide resolved
src/node_wasm_web_api.h Outdated Show resolved Hide resolved
src/node_wasm_web_api.h Outdated Show resolved Hide resolved
src/node_wasm_web_api.cc Outdated Show resolved Hide resolved
src/node_wasm_web_api.cc Outdated Show resolved Hide resolved
src/node_wasm_web_api.h Show resolved Hide resolved
@tniessen tniessen force-pushed the wasm-web-api branch 4 times, most recently from f76bda6 to 8689723 Compare Apr 13, 2022
@tniessen tniessen added the review wanted label Apr 17, 2022
@tniessen tniessen added the request-ci label Apr 17, 2022
@github-actions github-actions bot removed the request-ci label Apr 17, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

Copy link
Contributor

@aduh95 aduh95 left a comment

Should we add WTP tests? Maybe https://github.com/web-platform-tests/wpt/tree/HEAD/wasm/webapi or a subset of it?

doc/api/errors.md Show resolved Hide resolved
@tniessen
Copy link
Member Author

@tniessen tniessen commented Apr 18, 2022

Should we add WTP tests?

That's a good idea. It's not straightforward because many WPT tests for this API rely on fetch and we currently mock fetch in WPT.

Of the ones I can run easily, wasm/webapi/empty-body.any.js is failing, presumably due to nodejs/undici#1345.

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Apr 18, 2022

Should we add WTP tests?

That's a good idea. It's not straightforward because many WPT tests for this API rely on fetch and we currently mock fetch in WPT.

Of the ones I can run easily, wasm/webapi/empty-body.any.js is failing, presumably due to nodejs/undici#1345.

I think you can create a status file that lists all the failing tests, and that can be used as a TODO list for contributors who want to improve Node.js API compliance. But maybe that's a lot of work (I've never done that myself so not sure), and it probably should not block this from landing.

nodejs-github-bot pushed a commit that referenced this issue Apr 25, 2022
As per Section 3 of the WebAssembly Web API spec.

PR-URL: #42842
Refs: #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>
targos pushed a commit that referenced this issue Apr 28, 2022
Refs: #42660
Refs: #42701

PR-URL: #42836
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this issue Apr 28, 2022
As per Section 3 of the WebAssembly Web API spec.

PR-URL: #42842
Refs: #42701
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos added a commit that referenced this issue May 2, 2022
Notable changes:

doc:
  * add @kuriyosh to collaborators (Yoshiki Kurihara) #42824
lib,src:
  * (SEMVER-MINOR) implement WebAssembly Web API (Tobias Nießen) #42701
test_runner:
  * (SEMVER-MINOR) add initial CLI runner (Colin Ihrig) #42658
worker:
  * (SEMVER-MINOR) add hasRef() to MessagePort (Darshan Sen) #42849

PR-URL: #42943
@targos targos mentioned this pull request May 2, 2022
targos added a commit that referenced this issue May 3, 2022
Notable changes:

doc:
  * add @kuriyosh to collaborators (Yoshiki Kurihara) #42824
lib,src:
  * (SEMVER-MINOR) implement WebAssembly Web API (Tobias Nießen) #42701
test_runner:
  * (SEMVER-MINOR) add initial CLI runner (Colin Ihrig) #42658
worker:
  * (SEMVER-MINOR) add hasRef() to MessagePort (Darshan Sen) #42849

PR-URL: #42943
tniessen added a commit to tniessen/node that referenced this issue May 3, 2022
tniessen added a commit to tniessen/node that referenced this issue May 3, 2022
nodejs-github-bot pushed a commit that referenced this issue May 5, 2022
Refs: #42701
Refs: nodejs/undici#1346
Refs: #42939

PR-URL: #42960
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 9, 2022

Worth noting that this PR was a breaking change for Emscripten (see emscripten-core/emscripten#16913). In the future, we should remember to land this kind of addition as semver-major, as we do for introducing new globals.

@richardlau
Copy link
Member

@richardlau richardlau commented May 9, 2022

Worth noting that this PR was a breaking change for Emscripten (see emscripten-core/emscripten#16913). In the future, we should remember to land this kind of addition as semver-major, as we do for introducing new globals.

Should we revert?

@richardlau richardlau added dont-land-on-v14.x dont-land-on-v16.x dont-land-on-v17.x labels May 9, 2022
@richardlau
Copy link
Member

@richardlau richardlau commented May 9, 2022

I'll add dont-land-on labels for now, but perhaps we should relabel as semver-major?

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 9, 2022

I think it's fine to land it on v16.x, because it's behind the --experimental-fetch flag.

@aduh95 aduh95 removed the dont-land-on-v16.x label May 9, 2022
RafaelGSS pushed a commit that referenced this issue May 10, 2022
Refs: #42701
Refs: nodejs/undici#1346
Refs: #42939

PR-URL: #42960
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
rekmarks added a commit to MetaMask/snaps-skunkworks that referenced this issue May 14, 2022
Node landed `WebAssembly.compileStreaming` and `instantiateStreaming` in nodejs/node#42701 and made it available in Node 18, so we no longer need to exclude these properties from our WebAssembly endowment. This PR therefore removes the WebAssembly endowment factory, which will now be included in whatever form it exists on the execution environment root realm global.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready dont-land-on-v14.x dont-land-on-v17.x experimental fetch lib / src needs-ci notable-change semver-minor wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants