deps: add simdjson to node.js #47991

anonrig · 2023-05-13T19:31:09Z

This would remove all our json parsing code inside src folder and help us provide faster solutions. I'm happy to add an example implementation to this pull-request if requested.

cc @lemire

nodejs-github-bot · 2023-05-13T19:31:13Z

Review requested:

@nodejs/actions
@nodejs/gyp
@nodejs/tsc

marco-ippolito · 2023-05-13T19:38:18Z

LGTM on the tooling side

RafaelGSS

Can you include a specific path to use simdjson and provide benchmarks to hold your statement?

json parsing code inside src folder and help us provide faster solutions.

mscdex · 2023-05-13T23:47:24Z

I'm confused by this as well. What is wrong with using V8's built-in JSON parse function?

nodejs-github-bot · 2023-05-14T06:17:52Z

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

mcollina · 2023-05-14T06:18:19Z

I have a feeling that this will not run on all our supported platforms (kicked off CI to verify).

Ultimately, simdjson is not significantly faster than JSON.parse() if we need the full objects in JS-land (creating the objects is the bottleneck, not parsing them).

nodejs-github-bot · 2023-05-14T15:23:03Z

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

anonrig · 2023-05-14T17:55:46Z

I have a feeling that this will not run on all our supported platforms (kicked off CI to verify).

@mcollina It seems only AIX is failing. I opened an upstream issue and working on a fix for simdjson. (cc @lemire)

Ultimately, simdjson is not significantly faster than JSON.parse() if we need the full objects in JS-land (creating the objects is the bottleneck, not parsing them).

I don't have sufficient data to back my claim, but this might not be a valid argument, I'm not sure. There are other places on top of JSON.parse that can be beneficial for simdjson such as InternalModuleReadJSON which is used to speed up module loading. It's worth discussing if those places are sufficient enough to add a new dependency to Node.js. I'm interested in hearing your opinion.

I'm confused by this as well. What is wrong with using V8's built-in JSON parse function?

@mscdex Nothing is wrong with the built-in JSON parse. I'm experimenting and asking for community feedback before investing a lot of time in this.

Long story short:

A couple of months ago, in one of @joyeecheung's pull requests, I don't remember which one at the moment but I've communicated the idea of using simdjson to speed up certain functions. I'm interested in researching & experimenting with where we can leverage simdjson and improve performance while reducing our json specific implementations.

anonrig · 2023-05-14T21:59:36Z

Updated simdutf to v3.1.8 fixing AIX build error.

nodejs-github-bot · 2023-05-14T22:05:14Z

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

nodejs-github-bot · 2023-05-15T00:17:03Z

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

mcollina · 2023-05-15T07:20:25Z

I don't have sufficient data to back my claim, but this might not be a valid argument, I'm not sure. There are other places on top of JSON.parse that can be beneficial for simdjson such as InternalModuleReadJSON which is used to speed up module loading. It's worth discussing if those places are sufficient enough to add a new dependency to Node.js. I'm interested in hearing your opinion.

Ultimately it depends on what's the speedup in that operation. I'd restrain from adding a dependency without a merasurable improvement.

GeoffreyBooth · 2023-05-15T21:49:40Z

There are other places on top of JSON.parse that can be beneficial for simdjson such as InternalModuleReadJSON which is used to speed up module loading.

We read a lot of package.json files during module loading, especially for typical real-world apps with a hundred or more dependencies. If the library speeds that up, it should produce a measurable if not noticeable performance boost.

anonrig · 2023-05-23T00:47:27Z

@nodejs/loaders In a total of 5 tests are failing at the moment. Appreciate any help.

JakobJingleheimer

Sorry for the delay (this got buried in my inbox). It looks like (the good news is) all the failures here are related.

The error occurs after several checks against various packageJSON fields. I'm only familiar with the ESM side of modules, but I would bet your removal of packageJsonCache.set(jsonPath, filtered); is the cause (the reads against packageJSON fields within Module._resolveFilename (wherefrom the error is thrown) are probably trying to read from packageJsonCache, which no-longer gets set.

lib/internal/modules/esm/package_config.js

JakobJingleheimer · 2023-06-10T14:24:26Z

lib/internal/modules/package_json_reader.js

 * @param {string} jsonPath
 */
 function read(jsonPath) {
  if (cache.has(jsonPath)) {
    return cache.get(jsonPath);
  }

-  const { 0: string, 1: containsKeys } = internalModuleReadJSON(
+  const {
+    0: includes_keys,


nit: this is javascript, where camelCase is the standard

Suggested change

0: includes_keys,

0: includesKeys,

Definitely, maybe we should have an eslint rule about this in a different PR.

I'm surprised there isn't one @aduh95 I do you know if this has been discussed previously?

lib/internal/modules/package_json_reader.js

anonrig · 2023-06-15T18:00:10Z

I've reduced the failed tests to the following:

test/es-module/test-esm-exports.mjs
test/es-module/test-esm-exports-deprecations.mjs
test/pummel/test-policy-integrity-parent-commonjs.js

@RafaelGSS Any idea why test-policy-integrity-parent-commonjs.js is failing?

JakobJingleheimer

Sorry, I don't know anything about the remaining tests—they pre-date me and basically haven't been touched in years.

test/sequential/test-module-loading.js

lib/internal/modules/esm/package_config.js

JakobJingleheimer · 2023-06-15T20:09:51Z

lib/internal/modules/cjs/loader.js

-const packageJsonCache = new SafeMap();
-
 function readPackage(requestPath) {
  const jsonPath = path.resolve(requestPath, 'package.json');
-
-  const existing = packageJsonCache.get(jsonPath);
-  if (existing !== undefined) return existing;
-
-  const result = packageJsonReader.read(jsonPath);
-  const json = result.containsKeys === false ? '{}' : result.string;
-  if (json === undefined) {
-    packageJsonCache.set(jsonPath, false);
-    return false;
-  }
-
-  try {
-    const filtered = filterOwnProperties(JSONParse(json), [
-      'name',
-      'main',
-      'exports',
-      'imports',
-      'type',
-    ]);
-    packageJsonCache.set(jsonPath, filtered);
-    return filtered;
-  } catch (e) {
-    e.path = jsonPath;
-    e.message = 'Error parsing ' + jsonPath + ': ' + e.message;
-    throw e;
-  }


Oow, nice. It looks like this code was actually redundant to package_config.js / getPackageConfig()

Thanks! We were having 2 different caches and duplicated code for both in ESM and CJS.

Hmm actually was that a bug or a feature? 🤔

@guybedford do you know is CJS and ESM are supposed to have different package.json caches?

JakobJingleheimer · 2023-06-15T20:11:00Z

lib/internal/modules/package_json_reader.js

+ * Returns undefined for all failure cases.
 * @param {string} jsonPath
 */
 function read(jsonPath) {


It would be nice to include a @returns with the type and explanation.

I updated the JSDOC to the following. Let me know, if it is not sufficient:

/** * Returns undefined for all failure cases. * @param {string} jsonPath * @returns {{ * name?: string, * main?: string, * exports?: string | Record<string, unknown>, * imports?: string | Record<string, unknown>, * type: 'commonjs' | 'module' | 'none' | unknown, * } | undefined} */

Nit: I would move the Returns undefined for all failure cases. after the type for @returns (and merely say `undefined` on failure)

Also, I think the type for exports' & imports' Record value is known (I think it's a resolved URL['href']?), but maybe unknown is fine for now.

cjihrig

Since this has been open for longer than seven days and has a sign off, I'm going to request changes to prevent this from accidentally landing until benchmarks are provided.

anonrig · 2023-06-15T21:05:04Z

Since this has been open for longer than seven days and has a sign off, I'm going to request changes to prevent this from accidentally landing until benchmarks are provided.

Since there is a push after that review, it will never get merged without a new approved review.

cjihrig · 2023-06-15T21:08:36Z

Since there is a push after that review, it will never get merged without a new approved review.

To be honest, I don't think there should be an approval on this PR at all yet, so it's entirely possible that another premature approval could happen.

anonrig force-pushed the add-simdjson branch from 32ff0c9 to 59ad1e9 Compare May 13, 2023 19:32

This comment was marked as resolved.

Sign in to view

anonrig force-pushed the add-simdjson branch from 59ad1e9 to 617597d Compare May 13, 2023 19:36

anonrig requested a review from marco-ippolito May 13, 2023 19:36

marco-ippolito approved these changes May 13, 2023

View reviewed changes

anonrig force-pushed the add-simdjson branch from 617597d to 0c5c1de Compare May 13, 2023 19:37

RafaelGSS reviewed May 13, 2023

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 14, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 14, 2023

anonrig force-pushed the add-simdjson branch from 0c5c1de to 30ed750 Compare May 14, 2023 21:59

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label May 14, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 14, 2023

github-actions bot mentioned this pull request May 15, 2023

CI Reliability 2023-05-15 nodejs/reliability#566

Open

17 tasks

anonrig force-pushed the add-simdjson branch from 92235dc to 09ea411 Compare May 22, 2023 23:47

anonrig force-pushed the add-simdjson branch 2 times, most recently from 1138d7a to c62068c Compare May 23, 2023 02:33

anonrig added the loaders Issues and PRs related to ES module loaders label May 23, 2023

anonrig force-pushed the add-simdjson branch 2 times, most recently from 1be0fe4 to 05ed646 Compare May 23, 2023 20:44

anonrig mentioned this pull request Jun 1, 2023

Improve the performance of package json reader nodejs/performance#91

Closed

JakobJingleheimer reviewed Jun 10, 2023

View reviewed changes

anonrig force-pushed the add-simdjson branch 4 times, most recently from be620d3 to d1899e9 Compare June 15, 2023 00:17

anonrig requested review from GeoffreyBooth and JakobJingleheimer June 15, 2023 00:38

deps: add simdjson to node.js

b85a2b1

anonrig force-pushed the add-simdjson branch from d1899e9 to ec2bcdd Compare June 15, 2023 17:59

anonrig force-pushed the add-simdjson branch from ec2bcdd to 5067e5f Compare June 15, 2023 18:02

JakobJingleheimer reviewed Jun 15, 2023

View reviewed changes

cjihrig requested changes Jun 15, 2023

View reviewed changes

anonrig force-pushed the add-simdjson branch 2 times, most recently from a80c028 to 75f9437 Compare June 15, 2023 22:05

src: improve package.json reader performance

fe6b124

anonrig force-pushed the add-simdjson branch from 75f9437 to fe6b124 Compare June 16, 2023 02:17

anonrig mentioned this pull request Jun 16, 2023

lib: merge cjs and esm package json reader caches #48477

Merged

anonrig closed this Aug 13, 2023

anonrig deleted the add-simdjson branch August 13, 2023 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: add simdjson to node.js #47991

deps: add simdjson to node.js #47991

anonrig commented May 13, 2023 •

edited

Loading

nodejs-github-bot commented May 13, 2023

This comment was marked as resolved.

marco-ippolito commented May 13, 2023

RafaelGSS left a comment •

edited

Loading

mscdex commented May 13, 2023

nodejs-github-bot commented May 14, 2023

mcollina commented May 14, 2023

nodejs-github-bot commented May 14, 2023

anonrig commented May 14, 2023

anonrig commented May 14, 2023

nodejs-github-bot commented May 14, 2023

nodejs-github-bot commented May 15, 2023

mcollina commented May 15, 2023

GeoffreyBooth commented May 15, 2023

anonrig commented May 23, 2023

JakobJingleheimer left a comment

JakobJingleheimer Jun 10, 2023

anonrig Jun 10, 2023

JakobJingleheimer Jun 10, 2023

anonrig commented Jun 15, 2023 •

edited

Loading

JakobJingleheimer left a comment

JakobJingleheimer Jun 15, 2023

anonrig Jun 15, 2023

JakobJingleheimer Jun 16, 2023

JakobJingleheimer Jun 15, 2023

anonrig Jun 15, 2023

JakobJingleheimer Jun 16, 2023 •

edited

Loading

cjihrig left a comment

anonrig commented Jun 15, 2023

cjihrig commented Jun 15, 2023

deps: add simdjson to node.js #47991

deps: add simdjson to node.js #47991

Conversation

anonrig commented May 13, 2023 • edited Loading

nodejs-github-bot commented May 13, 2023

This comment was marked as resolved.

marco-ippolito commented May 13, 2023

RafaelGSS left a comment • edited Loading

Choose a reason for hiding this comment

mscdex commented May 13, 2023

nodejs-github-bot commented May 14, 2023

mcollina commented May 14, 2023

nodejs-github-bot commented May 14, 2023

anonrig commented May 14, 2023

anonrig commented May 14, 2023

nodejs-github-bot commented May 14, 2023

nodejs-github-bot commented May 15, 2023

mcollina commented May 15, 2023

GeoffreyBooth commented May 15, 2023

anonrig commented May 23, 2023

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonrig commented Jun 15, 2023 • edited Loading

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

anonrig commented Jun 15, 2023

cjihrig commented Jun 15, 2023

anonrig commented May 13, 2023 •

edited

Loading

RafaelGSS left a comment •

edited

Loading

anonrig commented Jun 15, 2023 •

edited

Loading

JakobJingleheimer Jun 16, 2023 •

edited

Loading