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

lib: merge cjs and esm package json reader caches by anonrig · Pull Request #48477 · nodejs/node · GitHub

/ node Public

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
ConversationCommits (9)ChecksFiles changed
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 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 commented Jun 16, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

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 force-pushed the refactor-package-json branch 3 times, most recently from d6a4186 to 24d52e6 Compare June 16, 2023 22:48
JakobJingleheimer approved these changes Jun 17, 2023
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM 🙌

Comment thread lib/internal/modules/package_json_reader.js Outdated
ljharb reviewed Jun 17, 2023
View reviewed changes
Comment thread lib/internal/modules/package_json_reader.js Outdated
arcanis reviewed Jun 17, 2023
View reviewed changes
Comment thread lib/internal/modules/cjs/loader.js Outdated
anonrig force-pushed the refactor-package-json branch from 24d52e6 to 7806a59 Compare June 17, 2023 21:22
JakobJingleheimer reviewed Jun 18, 2023
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good aside from the the new susceptibility to prototype pollution

Comment thread lib/internal/modules/cjs/loader.js Outdated
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 force-pushed the refactor-package-json branch from 7806a59 to 0b605b8 Compare June 18, 2023 16:37
GeoffreyBooth approved these changes Jun 19, 2023
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Nevermind, I see it in the top post 😄

anonrig added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jun 21, 2023
anonrig force-pushed the refactor-package-json branch from 0b605b8 to d4e2a4e Compare June 21, 2023 16:58

anonrig commented Jun 21, 2023

Copy link
Copy Markdown
Member Author

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

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2023

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 commented Jun 29, 2023

Copy link
Copy Markdown
Collaborator

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

GeoffreyBooth added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 29, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 29, 2023
nodejs-github-bot merged commit 951da52 into nodejs:main Jun 29, 2023

nodejs-github-bot commented Jun 29, 2023

Copy link
Copy Markdown
Collaborator

Landed in 951da52

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
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 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
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
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 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 mentioned this pull request Nov 11, 2023
Closed
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
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
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 mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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
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 mentioned this pull request Jan 15, 2025
Closed
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

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

Assignees

No one assigned

Labels

commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. 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.

Back | FazBrowse Home | New Git URL