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

anonrig · 2023-06-16T22:28:27Z

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 · 2023-06-16T22:28:31Z

Review requested:

@nodejs/loaders
@nodejs/modules

lib/internal/modules/package_json_reader.js

JakobJingleheimer

LGTM 🙌

lib/internal/modules/package_json_reader.js

lib/internal/modules/cjs/loader.js

JakobJingleheimer

Looks good aside from the the new susceptibility to prototype pollution

lib/internal/modules/cjs/loader.js

lib/internal/modules/esm/package_config.js

lib/internal/modules/esm/resolve.js

GeoffreyBooth

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

Nevermind, I see it in the top post 😄

anonrig · 2023-06-21T16:58:34Z

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

nodejs-github-bot · 2023-06-28T23:41:21Z

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

nodejs-github-bot · 2023-06-29T01:36:37Z

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

nodejs-github-bot · 2023-06-29T18:45:33Z

Landed in 951da52

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>

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 · 2023-09-11T02:22:37Z

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

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>

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>

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>

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

GeoffreyBooth reviewed Jun 17, 2023

View reviewed changes

lib/internal/modules/package_json_reader.js Outdated Show resolved Hide resolved

JakobJingleheimer approved these changes Jun 17, 2023

View reviewed changes

lib/internal/modules/package_json_reader.js Show resolved Hide resolved

ljharb reviewed Jun 17, 2023

View reviewed changes

lib/internal/modules/package_json_reader.js Show resolved Hide resolved

arcanis reviewed Jun 17, 2023

View reviewed changes

lib/internal/modules/cjs/loader.js Show resolved Hide resolved

anonrig force-pushed the refactor-package-json branch from 24d52e6 to 7806a59 Compare June 17, 2023 21:22

anonrig requested review from GeoffreyBooth and JakobJingleheimer June 17, 2023 21:23

GeoffreyBooth approved these changes Jun 17, 2023

View reviewed changes

JakobJingleheimer reviewed Jun 18, 2023

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

ljharb reviewed Jun 18, 2023

View reviewed changes

lib/internal/modules/esm/package_config.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/package_config.js Show resolved Hide resolved

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

anonrig force-pushed the refactor-package-json branch from 7806a59 to 0b605b8 Compare June 18, 2023 16:37

anonrig requested review from GeoffreyBooth, JakobJingleheimer and ljharb June 18, 2023 16:39

JakobJingleheimer approved these changes Jun 18, 2023

View reviewed changes

GeoffreyBooth approved these changes Jun 19, 2023

View reviewed changes

VoltrexKeyva approved these changes Jun 19, 2023

View reviewed changes

anonrig added author ready PRs that have at least one approval, no pending requests for changes, 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

lib: merge cjs and esm package json reader caches

d4e2a4e

anonrig force-pushed the refactor-package-json branch from 0b605b8 to d4e2a4e Compare June 21, 2023 16:58

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

GeoffreyBooth approved these changes Jun 29, 2023

View reviewed changes

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

anonrig mentioned this pull request Jun 29, 2023

lib: remove aix directory case for package reader #48605

Merged

This was referenced Jun 30, 2023

CI Reliability 2023-06-30 nodejs/reliability#603

Open

CI Reliability 2023-07-01 nodejs/reliability#604

Open

CI Reliability 2023-07-02 nodejs/reliability#605

Open

CI Reliability 2023-07-03 nodejs/reliability#606

Open

RafaelGSS mentioned this pull request Jul 3, 2023

v20.4.0 proposal #48643

Merged

This was referenced Jul 4, 2023

CI Reliability 2023-07-04 nodejs/reliability#607

Open

CI Reliability 2023-07-05 nodejs/reliability#608

Open

ruyadorno added the backport-requested-v18.x label Sep 11, 2023

targos mentioned this pull request Nov 11, 2023

[v18.x] Backport most ESM and customization hook changes #50669

Closed

targos added backported-to-v18.x and removed backport-requested-v18.x labels Nov 23, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

richardlau mentioned this pull request Jan 15, 2025

[v20.x backport] src: move package_json_reader cache to c++ #56590

Closed

Uh oh!

Conversation

anonrig commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 16, 2023

Uh oh!

Uh oh!

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GeoffreyBooth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig commented Jun 21, 2023

Uh oh!

nodejs-github-bot commented Jun 28, 2023

Uh oh!

nodejs-github-bot commented Jun 29, 2023

Uh oh!

nodejs-github-bot commented Jun 29, 2023

Uh oh!

ruyadorno commented Sep 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

anonrig commented Jun 16, 2023 •

edited

Loading

GeoffreyBooth left a comment •

edited

Loading