module: use Wasm CJS lexer when available by guybedford · Pull Request #35583 · nodejs/node · GitHub

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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Review requested:

@nodejs/modules

CI: https://ci.nodejs.org/job/node-test-pull-request/33515/

lgtm

Any ideas what the failing OS platforms have in common here? Could it be endian issues?

https://ci.nodejs.org/job/node-test-pull-request/33515/

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.

Any ideas what the failing OS platforms have in common here? Could it be endian issues?

https://ci.nodejs.org/job/node-test-pull-request/33515/

AIX and LinuxONE are big endian so quite possibly.
cc @miladfarca @nodejs/platform-aix @nodejs/platform-s390

CI: https://ci.nodejs.org/job/node-test-pull-request/33516/

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.

lgtm

PR-URL: #35583 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

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 lexer.js:
https://github.com/guybedford/cjs-module-lexer/blob/master/src/lexer.js#L36
Uint16Array is being used which would behave differently on LE and BE machines. Could you try using a DataView instead and force LE ordering? It would look like this:

...
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 npm run build && npm run test that would be a huge help in verifying the fix.

PR-URL: #35583 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

PR-URL: nodejs#35583 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

module: use Wasm CJS lexer when available

d84f850

nodejs-github-bot added build Issues and PRs related to build files or the CI. esm Issues and PRs related to the ECMAScript Modules implementation. labels Oct 10, 2020

fixup bootstrap modules test

59bf7dd

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2020

mcollina approved these changes Oct 10, 2020

View reviewed changes

gracefully handle wasm failures

6a7c435

mcollina approved these changes Oct 10, 2020

View reviewed changes

GeoffreyBooth approved these changes Oct 10, 2020

View reviewed changes

guybedford added a commit that referenced this pull request Oct 12, 2020

module: use Wasm CJS lexer when available

f3ce281

PR-URL: #35583 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

guybedford closed this Oct 12, 2020

MylesBorins mentioned this pull request Oct 12, 2020

Release plan - v14.x Maintenance LTS nodejs/Release#567

Closed

guybedford mentioned this pull request Oct 13, 2020

module: cjs-module-lexer@0.4.2 big endian fix #35634

Closed

3 tasks

MylesBorins pushed a commit that referenced this pull request Oct 14, 2020

module: use Wasm CJS lexer when available

cb2f6ff

PR-URL: #35583 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

MylesBorins mentioned this pull request Oct 14, 2020

v14.14.0 proposal #35648

Merged

MylesBorins pushed a commit that referenced this pull request Nov 3, 2020

module: use Wasm CJS lexer when available

58d1175

PR-URL: #35583 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

MylesBorins mentioned this pull request Nov 3, 2020

v12.20.0 proposal #35950

Merged

MylesBorins pushed a commit that referenced this pull request Nov 16, 2020

module: use Wasm CJS lexer when available

354f358

PR-URL: #35583 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

aduh95 mentioned this pull request Dec 7, 2020

lib: add WebAssembly to primordials #36433

Closed

3 tasks

joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021

module: use Wasm CJS lexer when available

4ffd126

PR-URL: nodejs#35583 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

module: use Wasm CJS lexer when available by guybedford · Pull Request #35583 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: use Wasm CJS lexer when available - #35583

module: use Wasm CJS lexer when available#35583guybedford wants to merge 3 commits intonodejs:masternodejs/node:masterfrom guybedford:wasm-lexerguybedford/node:wasm-lexerCopy head branch name to clipboard

Conversation

guybedford commented Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Oct 10, 2020

Uh oh!

nodejs-github-bot commented Oct 10, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

guybedford commented Oct 10, 2020

Uh oh!

guybedford commented Oct 10, 2020

Uh oh!

guybedford commented Oct 10, 2020

Uh oh!

richardlau commented Oct 10, 2020

Uh oh!

nodejs-github-bot commented Oct 10, 2020

Uh oh!

guybedford commented Oct 10, 2020

Uh oh!

richardlau commented Oct 10, 2020

Uh oh!

guybedford commented Oct 10, 2020

Uh oh!

guybedford commented Oct 10, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

guybedford commented Oct 12, 2020

Uh oh!

miladfarca commented Oct 12, 2020

Uh oh!

guybedford commented Oct 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

module: use Wasm CJS lexer when available#35583
guybedford wants to merge 3 commits into
nodejs:masterfrom
guybedford:wasm-lexer

guybedford commented Oct 10, 2020 •
edited
Loading