GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — lib: merge cjs and esm package json reader caches by anonrig · Pull Request #48477 · nodejs/node

Conversation

anonrig commented Jun 16, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nodejs-github-bot commented Jun 16, 2023

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/loaders
@nodejs/modules

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 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

Comment thread

lib/internal/modules/package_json_reader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

JakobJingleheimer approved these changes

Jun 17, 2023

View reviewed changes

JakobJingleheimer left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM 🙌

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/internal/modules/package_json_reader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

ljharb reviewed

Jun 17, 2023

View reviewed changes

Comment thread

lib/internal/modules/package_json_reader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

arcanis reviewed

Jun 17, 2023

View reviewed changes

Comment thread

lib/internal/modules/cjs/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

JakobJingleheimer left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/internal/modules/cjs/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

ljharb reviewed

Jun 18, 2023

View reviewed changes

Comment thread

lib/internal/modules/esm/package_config.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/modules/esm/package_config.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/modules/esm/resolve.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

GeoffreyBooth left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lin72h reacted with eyes emoji

All reactions

👀 1 reaction

VoltrexKeyva approved these changes

Jun 19, 2023

View reviewed changes

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


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

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Jun 29, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

GeoffreyBooth approved these changes

Jun 29, 2023

View reviewed changes

GeoffreyBooth added the commit-queue PRs queued for automated landing through the Commit Queue. label

Jun 29, 2023

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. 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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

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

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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

targos added backported-to-v18.x and removed backport-requested-v18.x labels

Nov 23, 2023

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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

targos mentioned this pull request

Nov 28, 2023

v18.19.0 release proposal #50953

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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

richardlau mentioned this pull request

Jan 15, 2025

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

Closed

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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

anonrig commented Jun 16, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

GeoffreyBooth left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.