Skip to content

src: move package_json_reader cache to c++ #50322

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

src: move package_json_reader cache to c++ #50322

wants to merge 4 commits into from

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Oct 21, 2023 •
edited
Loading

Co-authored by @lemire

Local Benchmark

My local benchmarks says that running vite --version is 5% faster

❯ hyperfine 'node ../sveltejs-realworld/node_modules/vite/dist/node/cli.js --version' 'out/Release/node ../sveltejs-realworld/node_modules/vite/dist/node/cli.js --version' -w 10
Benchmark 1: node ../sveltejs-realworld/node_modules/vite/dist/node/cli.js --version
  Time (mean ± σ):     101.4 ms ±   0.6 ms    [User: 96.6 ms, System: 10.8 ms]
  Range (min … max):   100.3 ms … 102.5 ms    28 runs

Benchmark 2: out/Release/node ../sveltejs-realworld/node_modules/vite/dist/node/cli.js --version
  Time (mean ± σ):      96.3 ms ±   0.5 ms    [User: 90.9 ms, System: 10.1 ms]
  Range (min … max):    95.6 ms …  98.1 ms    30 runs

Summary
  out/Release/node ../sveltejs-realworld/node_modules/vite/dist/node/cli.js --version ran
    1.05 ± 0.01 times faster than node ../sveltejs-realworld/node_modules/vite/dist/node/cli.js --version

TODOs

  • Move modules functionality to node_modules.cc
    • internalModuleReadJSON
  • Move caching of package_json_reader to C++
    • Make node_modules.cc snapshottable
  • Update run_main.js to just avoid constructing the whole object for the entry point.
    • We could even call containsModuleSyntax from C++
  • Move getPackageScopeConfig from package_config.js to C++ side, and cache it on node_modules.cc
  • Replace all packageJSONReader calls that looks for specific attribute to have equivalent C++ functions
  • Investigate if imports & exports JSONParse needs to be cached on JS

Caveats

  • ERR_INVALID_PACKAGE_CONFIG does not include JSON.Parse() error message anymore. This mainly because we're using simdjson to avoid JS allocation while parsing for performance reasons.
  • exports/imports without string, array or object value will now throw ERR_INVALID_PACKAGE_CONFIG instead of returning an invalid value.

Benchmarks

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 21, 2023

Review requested:

  • @nodejs/gyp
  • @nodejs/loaders
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 21, 2023
@anonrig anonrig force-pushed the package-json-cache branch 8 times, most recently from e462554 to ba0be9b Compare October 22, 2023 00:34
@GeoffreyBooth GeoffreyBooth mentioned this pull request Oct 22, 2023
Draft
@anonrig anonrig force-pushed the package-json-cache branch 4 times, most recently from 5158e35 to 2690c64 Compare October 23, 2023 19:16
joyeecheung
joyeecheung reviewed Oct 24, 2023
View reviewed changes
@anonrig anonrig force-pushed the package-json-cache branch 12 times, most recently from 2bfbcdb to 8309e95 Compare October 25, 2023 19:31
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jan 13, 2025
@anonrig @joyeecheung
deps: add simdjson
7845d55 
PR-URL: nodejs#50322
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jan 13, 2025
@anonrig @lemire @joyeecheung
src: move package resolver to c++
7b7500a 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#50322
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jan 13, 2025
@anonrig @joyeecheung
module: merge config with package_json_reader
0f2e413 
PR-URL: nodejs#50322
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jan 13, 2025
@anonrig @lemire @joyeecheung
src: move package resolver to c++
6e50795 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#50322
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jan 13, 2025
@anonrig @joyeecheung
module: merge config with package_json_reader
1b5abae 
PR-URL: nodejs#50322
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Jan 13, 2025
Closed
marco-ippolito pushed a commit that referenced this pull request Jan 22, 2025
@anonrig @marco-ippolito
deps: add simdjson
e954ef6 
PR-URL: #50322
Backport-PR-URL: #56590
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jan 22, 2025
@anonrig @lemire @marco-ippolito
src: move package resolver to c++
eb5f757 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: #50322
Backport-PR-URL: #56590
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jan 22, 2025
@anonrig @marco-ippolito
module: merge config with package_json_reader
5bf61c5 
PR-URL: #50322
Backport-PR-URL: #56590
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
marco-ippolito added a commit that referenced this pull request Jan 22, 2025
@marco-ippolito
2025-01-28, Version 20.18.3 'Iron' (LTS)
860ec67 
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
deps:
  * (SEMVER-MINOR) add simdjson (Yagiz Nizipli) #50322
doc:
  * add LJHarb to collaborators (Jordan Harband) #56132
  * enforce strict policy to semver-major releases (Rafael Gonzaga) #55732
  * add jazelly to collaborators (Jason Zhang) #55531
module:
  * (SEMVER-MINOR) merge config with `package_json_reader` (Yagiz Nizipli) #50322
src:
  * (SEMVER-MINOR) move package resolver to c++ (Yagiz Nizipli) #50322
tools:
  * fix root certificate updater (Richard Lau) #55681

PR-URL: #56699
@marco-ippolito marco-ippolito mentioned this pull request Jan 22, 2025
Merged
marco-ippolito added a commit that referenced this pull request Jan 22, 2025
@marco-ippolito
2025-01-28, Version 20.18.3 'Iron' (LTS)
820c710 
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
deps:
  * (SEMVER-MINOR) add simdjson (Yagiz Nizipli) #50322
doc:
  * add LJHarb to collaborators (Jordan Harband) #56132
  * enforce strict policy to semver-major releases (Rafael Gonzaga) #55732
  * add jazelly to collaborators (Jason Zhang) #55531
esm:
  * mark import attributes and JSON module as stable (Nicolò Ribaudo) #55333
module:
  * (SEMVER-MINOR) merge config with `package_json_reader` (Yagiz Nizipli) #50322
src:
  * (SEMVER-MINOR) move package resolver to c++ (Yagiz Nizipli) #50322
tools:
  * fix root certificate updater (Richard Lau) #55681

PR-URL: #56699
marco-ippolito added a commit that referenced this pull request Jan 22, 2025
@marco-ippolito
2025-01-28, Version 20.18.3 'Iron' (LTS)
ec8d195 
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
deps:
  * (SEMVER-MINOR) add simdjson (Yagiz Nizipli) #50322
doc:
  * add LJHarb to collaborators (Jordan Harband) #56132
  * enforce strict policy to semver-major releases (Rafael Gonzaga) #55732
  * add jazelly to collaborators (Jason Zhang) #55531
esm:
  * mark import attributes and JSON module as stable (Nicolò Ribaudo) #55333
module:
  * (SEMVER-MINOR) merge config with `package_json_reader` (Yagiz Nizipli) #50322
src:
  * (SEMVER-MINOR) move package resolver to c++ (Yagiz Nizipli) #50322
tools:
  * fix root certificate updater (Richard Lau) #55681

PR-URL: #56699
marco-ippolito pushed a commit that referenced this pull request Jan 23, 2025
@anonrig @marco-ippolito
deps: add simdjson
b83675f 
PR-URL: #50322
Backport-PR-URL: #56590
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jan 23, 2025
@anonrig @lemire @marco-ippolito
src: move package resolver to c++
9e3ed9d 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: #50322
Backport-PR-URL: #56590
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jan 23, 2025
@anonrig @marco-ippolito
module: merge config with package_json_reader
fa4ca5d 
PR-URL: #50322
Backport-PR-URL: #56590
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jan 23, 2025
@anonrig @joyeecheung
deps: add simdjson
94fe872 
PR-URL: nodejs#50322
Backport-PR-URL: nodejs#56590
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jan 23, 2025
@anonrig @lemire @joyeecheung
src: move package resolver to c++
50ad869 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#50322
Backport-PR-URL: nodejs#56590
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jan 23, 2025
@anonrig @joyeecheung
module: merge config with package_json_reader
40034af 
PR-URL: nodejs#50322
Backport-PR-URL: nodejs#56590
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Jan 24, 2025
Closed
@BridgeAR BridgeAR mentioned this pull request May 2, 2025
Open
@BridgeAR
Copy link
Member

BridgeAR commented May 2, 2025

@anonrig this seems to cause a major performance regression. I did not look deeper into this but it's causing test code to run about 10x slower when running the following example code: DataDog/dd-trace-js#5360

@anonrig
Copy link
Member Author

anonrig commented May 2, 2025 •
edited
Loading

@anonrig this seems to cause a major performance regression. I did not look deeper into this but it's causing test code to run about 10x slower when running the following example code: DataDog/dd-trace-js#5360

@BridgeAR Happy to take a look. Can you share a reproduction that doesn't require any external knowledge, and open a new issue and tag me?

@BridgeAR
Copy link
Member

BridgeAR commented May 2, 2025

I can't look into it closer at the moment. Since about 80% of all time is taken in Node.js when running the example, I guess it should be fine to check the source code to cause the problem.
It is actually just a redirected issue report, due to originally being reported in aws/aws-cdk#33576.

I'll open a issue about it though.

@BridgeAR BridgeAR mentioned this pull request May 2, 2025
Closed
@IlyasShabi IlyasShabi mentioned this pull request Jul 16, 2025
Merged
This was referenced Sep 8, 2025
Closed
Merged
@Tyoneb Tyoneb mentioned this pull request Oct 24, 2025
Open
@michaelsmithxyz michaelsmithxyz mentioned this pull request Oct 27, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lemire lemire lemire left review comments

@JonasBa JonasBa JonasBa left review comments

@ljharb ljharb ljharb approved these changes

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@aduh95 aduh95 aduh95 approved these changes

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

Assignees

No one assigned

Labels

backport-open-v20.x Indicate that the PR has an open backport c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.