lib,src: implement WebAssembly Web API #42701
Conversation
|
Review requested: |
src/node_wasm_web_api.cc
Outdated
| new WasmStreamingObject(env, args.This()); | ||
| } | ||
|
|
||
| void WasmStreamingObject::Push( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
f76bda6
to
8689723
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Should we add WTP tests? Maybe https://github.com/web-platform-tests/wpt/tree/HEAD/wasm/webapi or a subset of it?
That's a good idea. It's not straightforward because many WPT tests for this API rely on Of the ones I can run easily, |
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. |
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>
|
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? |
|
I'll add dont-land-on labels for now, but perhaps we should relabel as semver-major? |
|
I think it's fine to land it on v16.x, because it's behind the |
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>
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.
Yeap, it does, It lands fine; except for the bootstrap, tried to fix this by adding the binding make[1]: Leaving directory '/home/juanarbol/GitHub/node/test/node-api/test_worker_terminate_finalization/build' [487/5756]
/usr/bin/python3.10 tools/test.py --mode=release \
\
--skip-tests= \
default \
addons js-native-api node-api
=== release test-bootstrap-modules ===
Path: parallel/test-bootstrap-modules
node:assert:123
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: These modules were not loaded:
Internal Binding wasm_web_api
at Object.<anonymous> (/home/juanarbol/GitHub/node/test/parallel/test-bootstrap-modules.js:216:8)
at Module._compile (node:internal/modules/cjs/loader:1105:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
at node:internal/main/run_main_module:17:47 { |
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>
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>
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>
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>
Now that we have
fetch(), we may as well also haveWebAssembly.compileStreaming()andWebAssembly.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 thev8::WasmStreaminginstance from JavaScript.This attempts to be a spec-compliant implementation with no node-specific extensions.
Refs: #41749
Fixes: #21130