-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
module: use Wasm CJS lexer when available #35583
Conversation
Review requested:
|
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.
lgtm
Any ideas what the failing OS platforms have in common here? Could it be endian issues? |
I'm not sure we have any Wasm tests for Node.js, so it could also be general Wasm coverage issues even? |
I've added a simple check to gracefully fallback to the JS implementation if there are any issues running Wasm. |
AIX and LinuxONE are big endian so quite possibly. |
Looking at the failures, the Mac and Windows failures are both due to test flakes. The genuine failures of the Wasm execution only seem to be linuxone and aix AFAICT. Endianness shouldn't actually be an issue because Wasm is always little endian, so that's why I'm wondering if these are upstream Wasm issues. Not sure how to debug without access to one of these machines unfortunately. Just rerunning CI with the graceful fallback. As a perf-only change, it seems fine to skip Wasm for these machines. |
How was the WebAssembly generated? FWIW There is a known endian issue with emscripten (emscripten-core/emscripten#12387). |
The Wasm is generated with LLVM, but as I say the Wasm virtual machine itself is always designed to be little endian even on big endian hardware. Hence why I'm wondering if these are more upstream issues. |
CI is passing now though, so we are good to landing pending further approvals. |
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.
lgtm
PR-URL: #35583 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
Landed in f3ce281. |
Yes it seems to be due to endianness. wasm is LE enforced, however typed arrays in Javascript depend on the host endianness and are not portable, looking at the source of ...
while (i < len){
new DataView(outBuf16.buffer).setUint16(outBuf16.byteOffset + (2*i), src.charCodeAt(i++), true);
} |
@miladfarca thank you so much for finding this, that does sound like it then! I've created a PR with big endian support at nodejs/cjs-module-lexer#13. If anyone has the hardware to test this on with an |
PR-URL: #35583 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
PR-URL: #35583 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
PR-URL: #35583 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
PR-URL: nodejs#35583 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
This uses the Web Assembly version of the cjs-module-lexer whenever Web Assembly is available, avoiding the v8 compiler cold start that applies to the JS version, causing slowdowns for parsing multi-MB JS files cold as reported in #35574.
This will gracefully then fallback to the JS implementation when eg using the
--jitless
flag in Node.js.This is the same version of the cjs-module-lexer codebase, and is only a performance patch.
I've also included a benchmark for the parser as well for any future work here (eg linking the native C in directly at some point again).
I've also added a graceful fallback if there are any Wasm execution issues.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes