Skip to content

src: fix internalModuleStat v8 fast path #58054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 29, 2025

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Apr 27, 2025

v8 fast path was not triggering before this. I've updated the implementation and added proper test for it.

cc @nodejs/cpp-reviewers @nodejs/performance

@anonrig anonrig requested review from jasnell, targos and ronag April 27, 2025 17:45
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/security-wg

@nodejs-github-bot 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 Apr 27, 2025
@anonrig anonrig force-pushed the yagiz/internal-module-stat branch from b563e9e to aa493c2 Compare April 27, 2025 17:56
@anonrig 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. labels Apr 27, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 27, 2025
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Apr 27, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.19%. Comparing base (e0cf8ae) to head (aa493c2).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 57.14% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #58054   +/-   ##
=======================================
  Coverage   90.18%   90.19%           
=======================================
  Files         630      630           
  Lines      186393   186392    -1     
  Branches    36595    36592    -3     
=======================================
+ Hits       168103   168119   +16     
+ Misses      11090    11089    -1     
+ Partials     7200     7184   -16     
Files with missing lines Coverage Δ
lib/fs.js 98.26% <100.00%> (ø)
lib/internal/fs/promises.js 98.24% <100.00%> (ø)
lib/internal/modules/cjs/loader.js 98.26% <100.00%> (ø)
lib/internal/modules/esm/resolve.js 96.17% <ø> (-0.01%) ⬇️
lib/internal/modules/package_json_reader.js 99.36% <ø> (-0.01%) ⬇️
src/node_external_reference.h 100.00% <ø> (ø)
src/node_file.cc 76.94% <57.14%> (+0.21%) ⬆️

... and 26 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.

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 29, 2025
@nodejs-github-bot nodejs-github-bot merged commit 5ed1bcb into nodejs:main Apr 29, 2025
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 5ed1bcb

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label May 1, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/58054
✔  Done loading data for nodejs/node/pull/58054
----------------------------------- PR info ------------------------------------
Title      src: fix internalModuleStat v8 fast path (#58054)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     anonrig:yagiz/internal-module-stat -> nodejs:main
Labels     lib / src, author ready, needs-ci
Commits    1
 - src: fix internalModuleStat v8 fast path
Committers 1
 - Yagiz Nizipli <yagiz@nizipli.com>
PR-URL: https://github.com/nodejs/node/pull/58054
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/58054
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 27 Apr 2025 17:45:21 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/58054#pullrequestreview-2797706038
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/58054#pullrequestreview-2797709654
   ⚠  This PR was merged on Tue, 29 Apr 2025 17:53:13 GMT
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-04-27T18:10:22Z: https://ci.nodejs.org/job/node-test-pull-request/66486/
- Querying data for job/node-test-pull-request/66486/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/14777283946

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #58054
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #58054
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. 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
Development

Successfully merging this pull request may close these issues.

5 participants