FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: move package_json_reader cache to c++ by anonrig · Pull Request #50322 · nodejs/node · GitHub

/ node Public

src: move package_json_reader cache to c++ - #50322

Closed
anonrig wants to merge 4 commits into
nodejs:mainfrom
anonrig:package-json-cache
ConversationCommits (4)ChecksFiles changed
Closed

src: move package_json_reader cache to c++#50322
anonrig wants to merge 4 commits into
nodejs:mainfrom
anonrig:package-json-cache

Conversation

anonrig commented Oct 21, 2023 •
edited
Loading

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

nodejs-github-bot commented Oct 21, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

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

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 force-pushed the package-json-cache branch 8 times, most recently from e462554 to ba0be9b Compare October 22, 2023 00:34
GeoffreyBooth mentioned this pull request Oct 22, 2023
Closed
anonrig force-pushed the package-json-cache branch 4 times, most recently from 5158e35 to 2690c64 Compare October 23, 2023 19:16
joyeecheung reviewed Oct 24, 2023
View reviewed changes
Comment thread src/node_modules.cc Outdated
anonrig force-pushed the package-json-cache branch 12 times, most recently from 2bfbcdb to 8309e95 Compare October 25, 2023 19:31

RafaelGSS commented Nov 29, 2023

Copy link
Copy Markdown
Member

@anonrig See #50954 (review).

RafaelGSS commented Nov 29, 2023

Copy link
Copy Markdown
Member

Adding dont-land labels again.

targos mentioned this pull request Dec 4, 2023
Merged

anonrig commented Dec 4, 2023

Copy link
Copy Markdown
Member Author

The mentioned issue is fixed in #50965

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)

RafaelGSS mentioned this pull request Dec 15, 2023
Merged
joyeecheung mentioned this pull request Dec 18, 2023
Merged

marco-ippolito commented Apr 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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

This was referenced May 1, 2024
Merged
Merged
marco-ippolito mentioned this pull request Jun 17, 2024
Merged
4 tasks
This was referenced Jun 17, 2024
Closed
Closed
anonrig mentioned this pull request Jun 18, 2024
Closed
This was referenced Oct 29, 2024
Closed
Merged
Merged
joyeecheung mentioned this pull request Jan 13, 2025
Closed
marco-ippolito mentioned this pull request Jan 22, 2025
Merged
joyeecheung mentioned this pull request Jan 24, 2025
Closed
BridgeAR mentioned this pull request May 2, 2025
Closed

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

anonrig commented May 2, 2025 •
edited
Loading

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?

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.

BridgeAR mentioned this pull request May 2, 2025
Closed
IlyasShabi mentioned this pull request Jul 16, 2025
Merged
This was referenced Sep 8, 2025
Closed
Merged
Tyoneb mentioned this pull request Oct 24, 2025
Closed
michaelsmithxyz mentioned this pull request Oct 27, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lemire lemire left review comments
ljharb ljharb approved these changes
mcollina mcollina approved these changes
jasnell jasnell approved these changes
GeoffreyBooth GeoffreyBooth approved these changes
JakobJingleheimer JakobJingleheimer approved these changes
aduh95 aduh95 approved these changes
joyeecheung Awaiting requested review from joyeecheung
+1 more reviewer
JonasBa JonasBa left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

Back | FazBrowse Home | New Git URL