Skip to content

module: improve getPackageType performance - #57599

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
dario-piotrowicz:dario/add-getPackageType-module-binding
Apr 6, 2025
Merged

module: improve getPackageType performance#57599
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
dario-piotrowicz:dario/add-getPackageType-module-binding

Conversation

@dario-piotrowicz

@dario-piotrowiczdario-piotrowicz commented Mar 23, 2025

Copy link
Copy Markdown
Member

packageJsonReader#getPackageType currently gets the whole package scope config from the C++ binding and then extracts its type field, my understanding is that this is wasteful (since the whole object needs to be serialized to cross the C++/JS boundary) and that gathering the type in C++ and returning only that would be more efficient, that's what I'm attempting to do in this PR 🙂 (I hope my C++ makes sense as I am pretty inexperienced with it 🤞)

@nodejs-github-bot

nodejs-github-bot commented Mar 23, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. typings labels Mar 23, 2025
@dario-piotrowicz
dario-piotrowiczforce-pushed the dario/add-getPackageType-module-binding branch from 759229d to 2c75280CompareMarch 23, 2025 21:06
@anonrig

anonrig commented Mar 23, 2025

Copy link
Copy Markdown
Member

I'll review tomorrow morning.

@codecov

codecovBot commented Mar 23, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.22%. Comparing base (1123585) to head (d242ecf).
Report is 70 commits behind head on main.

Files with missing linesPatch %Lines
src/node_modules.cc91.66%0 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #57599 +/- ##
==========================================
- Coverage 90.22% 90.22% -0.01% 
==========================================
Files 630 630 Lines 185055 185064 +9 Branches 36216 36223 +7 ==========================================
- Hits 166975 166969 -6 - Misses 11042 11043 +1 - Partials 7038 7052 +14
Files with missing linesCoverage Δ
lib/internal/modules/package_json_reader.js99.39% <100.00%> (ø)
src/node_modules.h100.00% <ø> (ø)
src/node_modules.cc78.73% <91.66%> (-0.29%)⬇️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dario-piotrowicz
dario-piotrowiczforce-pushed the dario/add-getPackageType-module-binding branch 3 times, most recently from 2052d4a to 3406431CompareMarch 24, 2025 00:15
jasnell
jasnell reviewed Mar 24, 2025
View reviewed changes
Comment threadsrc/node_modules.cc Outdated
jasnell
jasnell reviewed Mar 24, 2025
View reviewed changes
Comment threadsrc/node_modules.cc Outdated
@dario-piotrowicz
dario-piotrowiczforce-pushed the dario/add-getPackageType-module-binding branch from 3406431 to 0670e6eCompareMarch 24, 2025 23:38
@dario-piotrowicz
dario-piotrowiczforce-pushed the dario/add-getPackageType-module-binding branch from 0670e6e to 46649e7CompareMarch 24, 2025 23:45
jasnell
jasnell reviewed Mar 25, 2025
View reviewed changes
Comment threadsrc/node_modules.cc Outdated
@dario-piotrowicz@jasnell
Apply suggestions from code review
bd15451
Co-authored-by: James M Snell <jasnell@gmail.com>
@jasnell

jasnell commented Mar 25, 2025

Copy link
Copy Markdown
Member

I think this should be ready to go but we're unable to run jenkins CI on it currently due to some ongoing maintenance, Once the CI system is unrestricted again we'll get the CI run scheduled so we can get this landed.

@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 25, 2025
anonrig
anonrig approved these changes Mar 28, 2025
View reviewed changes
Comment threadlib/internal/modules/package_json_reader.js
mcollina
mcollina approved these changes Mar 31, 2025
View reviewed changes

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

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Apr 1, 2025

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 5, 2025

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 5, 2025

Copy link
Copy Markdown
Collaborator

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

@aduh95aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 6, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 6, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 8c25465 into nodejs:mainApr 6, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 6, 2025

Copy link
Copy Markdown
Collaborator

Landed in 8c25465

@dario-piotrowicz
dario-piotrowicz deleted the dario/add-getPackageType-module-binding branch April 6, 2025 17:10
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@dario-piotrowicz@RafaelGSS
module: improve getPackageType performance
beb21a0
PR-URL: #57599
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@dario-piotrowicz@RafaelGSS
module: improve getPackageType performance
a540689
PR-URL: #57599
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@dario-piotrowicz@aduh95
module: improve getPackageType performance
94a065d
PR-URL: #57599
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@dario-piotrowicz@RafaelGSS
module: improve getPackageType performance
9597bf6
PR-URL: #57599
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
@dario-piotrowicz@aduh95
module: improve getPackageType performance
026fe5e
PR-URL: #57599
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@dario-piotrowicz@aduh95
module: improve getPackageType performance
0bdf6ec
PR-URL: #57599
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
@github-actionsgithub-actionsBot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
@dario-piotrowicz@aduh95
module: improve getPackageType performance
bb09b4d
PR-URL: #57599
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
@ghostghost mentioned this pull request Jun 8, 2025
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell left review comments
@mcollinamcollinamcollina approved these changes
@anonriganonriganonrig approved these changes
@UlisesGasconUlisesGasconUlisesGascon approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.moduleIssues and PRs related to the module subsystem.needs-ciPRs that need a full CI run.typings

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@dario-piotrowicz@nodejs-github-bot@anonrig@jasnell@mcollina@UlisesGascon@aduh95