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
anonrig wants to merge 4 commits into from
Closed

src: move package_json_reader cache to c++ #50322

anonrig wants to merge 4 commits into from

Conversation

@anonrig
Copy link
Member

@anonriganonrig 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-botnodejs-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
@anonriganonrigforce-pushed the package-json-cache branch 8 times, most recently from e462554 to ba0be9bCompareOctober 22, 2023 00:34
@GeoffreyBoothGeoffreyBooth mentioned this pull request Oct 22, 2023
Draft
@anonriganonrigforce-pushed the package-json-cache branch 4 times, most recently from 5158e35 to 2690c64CompareOctober 23, 2023 19:16
joyeecheung
joyeecheung reviewed Oct 24, 2023
View reviewed changes
@anonriganonrigforce-pushed the package-json-cache branch 12 times, most recently from 2bfbcdb to 8309e95CompareOctober 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]>
@joyeecheungjoyeecheung 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-ippolitomarco-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]>
@joyeecheungjoyeecheung mentioned this pull request Jan 24, 2025
Closed
@BridgeARBridgeAR 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
MemberAuthor

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.

@BridgeARBridgeAR mentioned this pull request May 2, 2025
Closed
@IlyasShabiIlyasShabi mentioned this pull request Jul 16, 2025
Merged
This was referenced Sep 8, 2025
Closed
Merged
@TyonebTyoneb mentioned this pull request Oct 24, 2025
Open
@michaelsmithxyzmichaelsmithxyz mentioned this pull request Oct 27, 2025
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lemirelemirelemire left review comments

@JonasBaJonasBaJonasBa left review comments

@ljharbljharbljharb approved these changes

@mcollinamcollinamcollina approved these changes

@jasnelljasnelljasnell approved these changes

@GeoffreyBoothGeoffreyBoothGeoffreyBooth approved these changes

@JakobJingleheimerJakobJingleheimerJakobJingleheimer approved these changes

@aduh95aduh95aduh95 approved these changes

@joyeecheungjoyeecheungAwaiting requested review from joyeecheung

Assignees

No one assigned

Labels

backport-open-v20.xIndicate that the PR has an open backportc++Issues and PRs that require attention from people who are familiar with C++.commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.notable-changePRs with changes that should be highlighted in changelogs.semver-minorPRs 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.

16 participants

@anonrig@nodejs-github-bot@lemire@joyeecheung@StefanStojanovic@GeoffreyBooth@lpinca@RafaelGSS@marco-ippolito@BridgeAR@ljharb@mcollina@jasnell@JakobJingleheimer@JonasBa@aduh95