Visitar URL original
lib: merge cjs and esm package json reader caches by anonrig · Pull Request #48477 · nodejs/node · GitHub
Skip to content

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

Merged
nodejs-github-bot merged 9 commits into
nodejs:mainfrom
anonrig:refactor-package-json
Jun 29, 2023
Merged

lib: merge cjs and esm package json reader caches#48477
nodejs-github-bot merged 9 commits into
nodejs:mainfrom
anonrig:refactor-package-json

Conversation

@anonrig

@anonrig anonrig commented Jun 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented Jun 16, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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
Comment thread lib/internal/modules/package_json_reader.js Outdated
JakobJingleheimer
JakobJingleheimer approved these changes Jun 17, 2023
View reviewed changes

@JakobJingleheimer JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 🙌

Comment thread lib/internal/modules/package_json_reader.js Outdated
ljharb
ljharb reviewed Jun 17, 2023
View reviewed changes
Comment thread lib/internal/modules/package_json_reader.js Outdated
arcanis
arcanis reviewed Jun 17, 2023
View reviewed changes
Comment thread lib/internal/modules/cjs/loader.js Outdated
@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

@JakobJingleheimer JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

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

Comment thread lib/internal/modules/cjs/loader.js Outdated
ljharb
ljharb reviewed Jun 18, 2023
View reviewed changes
Comment thread lib/internal/modules/esm/package_config.js Outdated
Comment thread lib/internal/modules/esm/package_config.js Outdated
Comment thread lib/internal/modules/esm/resolve.js Outdated
@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

@GeoffreyBooth GeoffreyBooth left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see a bunch of C++ got removed, should we do a benchmark?

Nevermind, I see it in the top post 😄

@anonrig anonrig added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Jun 21, 2023
@anonrig
anonrig force-pushed the refactor-package-json branch from 0b605b8 to d4e2a4e Compare June 21, 2023 16:58
@anonrig

anonrig commented Jun 21, 2023

Copy link
Copy Markdown
Member Author

I fixed the conflict, and force pushed it. Appreciate reviewing it again.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 28, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52547/

@nodejs-github-bot

nodejs-github-bot commented Jun 29, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52548/

@GeoffreyBooth GeoffreyBooth added the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 29, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 29, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 951da52 into nodejs:main Jun 29, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 29, 2023

Copy link
Copy Markdown
Collaborator

Landed in 951da52

@anonrig anonrig mentioned this pull request Jun 29, 2023
Merged
This was referenced Jun 30, 2023
Open
Open
Open
Open
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@anonrig @RafaelGSS
lib: merge cjs and esm package json reader caches
15530fe 
PR-URL: #48477
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
This was referenced Jul 4, 2023
Open
Open
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
lib: merge cjs and esm package json reader caches
887d85c 
PR-URL: nodejs#48477
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
lib: merge cjs and esm package json reader caches
ce22b0e 
PR-URL: nodejs#48477
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@ruyadorno

ruyadorno commented Sep 11, 2023

Copy link
Copy Markdown
Member

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

@targos targos mentioned this pull request Nov 11, 2023
Closed
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
@anonrig @targos
lib: merge cjs and esm package json reader caches
a600822 
PR-URL: nodejs#48477
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Nov 23, 2023
@anonrig @targos
lib: merge cjs and esm package json reader caches
fe26f8a 
PR-URL: #48477
Backport-PR-URL: #50669
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos targos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
lib: merge cjs and esm package json reader caches
1769bf3 
PR-URL: nodejs/node#48477
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
lib: merge cjs and esm package json reader caches
e945f5e 
PR-URL: nodejs/node#48477
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@richardlau richardlau mentioned this pull request Jan 15, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardlau richardlau richardlau left review comments
@RafaelGSS RafaelGSS RafaelGSS left review comments
@mcollina mcollina mcollina approved these changes
@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
@ljharb ljharb Awaiting requested review from ljharb
@VoltrexKeyva VoltrexKeyva Awaiting requested review from VoltrexKeyva
@JakobJingleheimer JakobJingleheimer Awaiting requested review from JakobJingleheimer
@aduh95 aduh95 Awaiting requested review from aduh95
+1 more reviewer
@arcanis arcanis arcanis left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

commit-queue-squash PRs the Commit Queue should land as one squashed commit. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.