10BC0 lib: merge cjs and esm package json reader caches by anonrig · Pull Request #48477 · nodejs/node · GitHub
[go: up one dir, main page]
Skip to content

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

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
Merged
nodejs-github-bot merged 9 commits into from
Jun 29, 2023
Merged

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

nodejs-github-bot merged 9 commits into from
Jun 29, 2023

Conversation

@anonrig
Copy link
Member
@anonrig anonrig commented Jun 16, 2023 •
edited
Loading

This PR contains a refactor to merge package.json reader caches in both CJS and ESM.

I extracted the relevant parts from #47991.

cc @nodejs/loaders @nodejs/modules

PS: This PR has no effect on execution speed.

hyperfine './out/Release/node ../fastify/fastify.js' 'node ../fastify/fastify.js' 'bun ../fastify/fastify.js' -i --warmup 10

Benchmark 1: ./out/Release/node ../fastify/fastify.js
  Time (mean ± σ):      88.3 ms ±   1.8 ms    [User: 85.5 ms, System: 20.3 ms]
  Range (min … max):    84.7 ms …  94.8 ms    33 runs

Benchmark 2: node ../fastify/fastify.js
  Time (mean ± σ):      88.8 ms ±   2.1 ms    [User: 83.2 ms, System: 21.8 ms]
  Range (min … max):    84.0 ms …  95.0 ms    33 runs

Benchmark 3: bun ../fastify/fastify.js
  Time (mean ± σ):      99.6 ms ±   1.5 ms    [User: 84.5 ms, System: 19.9 ms]
  Range (min … max):    98.3 ms … 103.6 ms    29 runs

Summary
  ./out/Release/node ../fastify/fastify.js ran
    1.01 ± 0.03 times faster than node ../fastify/fastify.js
    1.13 ± 0.03 times faster than bun ../fastify/fastify.js

@nodejs-github-bot
Copy link
Collaborator
nodejs-github-bot commented Jun 16, 2023

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added 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 Jun 16, 2023
@anonrig anonrig force-pushed the refactor-package-json branch 3 times, most recently from d6a4186 to 24d52e6 Compare June 16, 2023 22:48
GeoffreyBooth
GeoffreyBooth reviewed Jun 17, 2023
View reviewed changes
JakobJingleheimer
JakobJingleheimer approved these changes Jun 17, 2023
View reviewed changes
Copy link
Member
@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🙌

ljharb
ljharb reviewed Jun 17, 2023
View reviewed changes
arcanis
arcanis reviewed Jun 17, 2023
View reviewed changes
@anonrig anonrig force-pushed the refactor-package-json branch from 24d52e6 to 7806a59 Compare June 17, 2023 21:22
JakobJingleheimer
JakobJingleheimer reviewed Jun 18, 2023
View reviewed changes
Copy link
Member
@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good aside from the the new susceptibility to prototype pollution

ljharb
ljharb reviewed Jun 18, 2023
View reviewed changes
@anonrig anonrig force-pushed the refactor-package-json branch from 7806a59 to 0b605b8 Compare June 18, 2023 16:37
GeoffreyBooth
GeoffreyBooth approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member
@GeoffreyBooth GeoffreyBooth left a comment •
edited