GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: move package_json_reader cache to c++ by anonrig · Pull Request #50322 · nodejs/node

Conversation

@anonrig

@anonrig anonrig commented Oct 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

  • Startup bench: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1468

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JakobJingleheimer, Mike-Dax, lachlancollins, munepom, and ronjouch reacted with hooray emoji bricss, alvarlagerlof, theoludwig, ChristoPy, CanadaHonk, twlite, JerrysShan, RomainLanz, Thanaen, shrujalshah28, and 7 more reacted with eyes emoji
All reactions
  • 🎉 5 reactions
  • 👀 17 reactions
@nodejs-github-bot

nodejs-github-bot commented Oct 21, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/loaders
  • @nodejs/startup
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 involving general changes in the lib/ or src/ directories. 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
[DO NOT MERGE] test: duplicate async_hooks tests in esm #44323
Closed
@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
Comment thread src/node_modules.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig force-pushed the package-json-cache branch 12 times, most recently from 2bfbcdb to 8309e95 Compare October 25, 2023 19:31
@RafaelGSS

RafaelGSS commented Nov 29, 2023

Copy link
Copy Markdown
Member

@anonrig See #50954 (review).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS

RafaelGSS commented Nov 29, 2023

Copy link
Copy Markdown
Member

Adding dont-land labels again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos targos mentioned this pull request Dec 4, 2023
src: omit non-string values of package.json main field #50965
Merged
@anonrig

anonrig commented Dec 4, 2023

Copy link
Copy Markdown
Member Author

The mentioned issue is fixed in #50965
lemire reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS

RafaelGSS commented Dec 15, 2023

Copy link
Copy Markdown
Member

Applying semver-minor PRs that contain new features and should be released in the next minor version. because we're adding a new dependency to Node.js (0dd53da)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS mentioned this pull request Dec 15, 2023
v21.5.0 proposal #51166
Merged
@joyeecheung joyeecheung mentioned this pull request Dec 18, 2023
module,win: fix long path resolve #51097
Merged
@marco-ippolito

marco-ippolito commented Apr 29, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@anonrig I would like to land this on v20 but its not landing cleanly, can you backport?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced May 1, 2024
doc: fix simdjson wrong link #51177
Merged
deps: update simdjson to 3.8.0 #52124
Merged
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
cli: implement node --run <script-in-package-json> #52190
Merged
4 tasks done
This was referenced Jun 17, 2024
module: support require()ing synchronous ESM graphs #51977
Closed
[v20.x] backport require(esm) and related patches #53500
Closed
@anonrig anonrig mentioned this pull request Jun 18, 2024
[v20.x backport] src: move package_json_reader cache to c++ #53502
Closed
This was referenced Oct 29, 2024
AddressSanitizer: container-overflow during package.json resolution with simdjson #55584
Closed
chore: move main entry to top tj/node-cookie-signature#51
Merged
chore: bump Node.js to v22.9.0 electron/electron#44281
Merged
@joyeecheung joyeecheung mentioned this pull request Jan 13, 2025
[v20.x backport] src: move package_json_reader cache to c++ #56590
Closed
@marco-ippolito marco-ippolito mentioned this pull request Jan 22, 2025
v20.18.3 proposal #56699
Merged
@joyeecheung joyeecheung mentioned this pull request Jan 24, 2025
[v20.x] backport unflagging of require(esm), part 1 (of 4?) #56730
Closed
@BridgeAR BridgeAR mentioned this pull request May 2, 2025
[BUG]: Severe performance degradation on Node 22 DataDog/dd-trace-js#5360
Closed
@BridgeAR

BridgeAR commented May 2, 2025

Copy link
Copy Markdown
Member

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented May 2, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented May 2, 2025

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR BridgeAR mentioned this pull request May 2, 2025
Significant performance regression with C++ package_json_reader cache #58126
Closed
@IlyasShabi IlyasShabi mentioned this pull request Jul 16, 2025
src: add cache to nearest parent package json #59086
Merged
This was referenced Sep 8, 2025
Significant heap usage regression in Node 22.19.0 #59733
Closed
src: reduce the nearest parent package JSON cache size #59888
Merged
@Tyoneb Tyoneb mentioned this pull request Oct 24, 2025
Perf regression in Node 22/24 when loading JS files #60397
Closed
@michaelsmithxyz michaelsmithxyz mentioned this pull request Oct 27, 2025
src: cache missing package.json files in the C++ package config cache #60425
Merged
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
@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
+1 more reviewer
@JonasBa JonasBa JonasBa left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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.

Uh oh!

There was an error while loading. Please reload this page.

16 participants

@anonrig @nodejs-github-bot @lemire @joyeecheung @StefanStojanovic @GeoffreyBooth @lpinca @RafaelGSS @marco-ippolito @BridgeAR @ljharb @mcollina @jasnell @JakobJingleheimer @JonasBa @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.