Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
module: use Wasm CJS lexer when available#35583
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
guybedford commented Oct 10, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Oct 10, 2020
Review requested:
|
nodejs-github-bot commented Oct 10, 2020
mcollina left a comment
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
guybedford commented Oct 10, 2020
Any ideas what the failing OS platforms have in common here? Could it be endian issues? |
guybedford commented Oct 10, 2020
I'm not sure we have any Wasm tests for Node.js, so it could also be general Wasm coverage issues even? |
guybedford commented Oct 10, 2020
I've added a simple check to gracefully fallback to the JS implementation if there are any issues running Wasm. |
richardlau commented Oct 10, 2020
AIX and LinuxONE are big endian so quite possibly. |
nodejs-github-bot commented Oct 10, 2020
guybedford commented Oct 10, 2020
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. |
richardlau commented Oct 10, 2020
How was the WebAssembly generated? FWIW There is a known endian issue with emscripten (emscripten-core/emscripten#12387). |
guybedford commented Oct 10, 2020
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. |
guybedford commented Oct 10, 2020
CI is passing now though, so we are good to landing pending further approvals. |
mcollina left a comment
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]>
guybedford commented Oct 12, 2020
Landed in f3ce281. |
miladfarca commented Oct 12, 2020
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){newDataView(outBuf16.buffer).setUint16(outBuf16.byteOffset+(2*i),src.charCodeAt(i++),true);} |
guybedford commented Oct 13, 2020
@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
--jitlessflag 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