GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: improve globSync performance by Trott · Pull Request #57725 · nodejs/node

Conversation

@Trott

@Trott Trott commented Apr 2, 2025

Copy link
Copy Markdown
Member

Benchmark results:

                                                                                    confidence improvement accuracy (*)   (**)  (***)
fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='lib' n=1000                    -0.28 %       ±2.63% ±3.50% ±4.56%
fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='lib' n=1000                     0.15 %       ±2.85% ±3.81% ±4.98%
fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='lib' n=1000                -1.03 %       ±2.85% ±3.79% ±4.94%
fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='lib' n=1000                     -0.49 %       ±1.39% ±1.86% ±2.42%
fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='lib' n=1000               *      0.82 %       ±0.68% ±0.91% ±1.19%
fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='lib' n=1000         ***      1.38 %       ±0.73% ±0.98% ±1.29%
fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='lib' n=1000                      0.31 %       ±2.33% ±3.09% ±4.03%
fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='lib' n=1000                     -0.90 %       ±3.45% ±4.59% ±5.98%
fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='lib' n=1000                  0.26 %       ±2.46% ±3.28% ±4.26%
fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='lib' n=1000                       0.01 %       ±0.92% ±1.23% ±1.60%
fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='lib' n=1000                       0.57 %       ±0.62% ±0.83% ±1.09%
fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='lib' n=1000           **      1.89 %       ±1.35% ±1.82% ±2.41%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 12 comparisons, you can thus expect the following amount of false-positive results:
  0.60 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.12 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@Trott
fs: improve globSync performance
7367a0b
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Apr 2, 2025
cjihrig
cjihrig approved these changes Apr 2, 2025
View reviewed changes
@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. author ready PRs with CI started, the required approvals, and no outstanding review comments. labels Apr 2, 2025
@codecov

codecov Bot commented Apr 2, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (1c2d98d) to head (7367a0b).
Report is 551 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57725      +/-   ##
==========================================
- Coverage   90.22%   90.22%   -0.01%     
==========================================
  Files         630      630              
  Lines      185073   185070       -3     
  Branches    36222    36223       +1     
==========================================
- Hits       166990   166982       -8     
+ Misses      11044    11041       -3     
- Partials     7039     7047       +8
Files with missing lines Coverage Δ
lib/internal/fs/glob.js 91.97% <100.00%> (-0.04%) ⬇️

... and 28 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 2, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65960/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow approved these changes Apr 2, 2025
View reviewed changes
anonrig
anonrig reviewed Apr 2, 2025
View reviewed changes
Comment thread lib/internal/fs/glob.js
// if it matches be more performant in the typical use case? #isExcluded()
// is often ()=>false which is about as optimizable as a function gets.
if (this.#cache.statSync(fullpath).isDirectory() && this.#isExcluded(`${fullpath}/`)) {
if (this.#isExcluded(`${fullpath}/`) && this.#cache.statSync(fullpath).isDirectory()) {

@anonrig anonrig Apr 2, 2025

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 think the fastest way to would be to use this function: https://github.com/nodejs/node/blob/main/src/node_file.cc#L1053

Particularly: internalBinding('fs').internalModuleStat(path) which avoids creating unnecessary Dir/Dirent objects.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow reacted with rocket emoji Trott reacted with eyes emoji
All reactions
  • 🚀 1 reaction
  • 👀 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65980/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit b2ab1a2 into nodejs:main Apr 7, 2025
@aduh95

aduh95 commented Apr 7, 2025

Copy link
Copy Markdown
Contributor

Landed in b2ab1a2
All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented Apr 7, 2025

Copy link
Copy Markdown
Member Author

I'll take a look at @anonrig's suggestion for a subsequent PR (unless someone beats me to it!).
anonrig reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott deleted the glob-perf branch April 7, 2025 22:49
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@Trott @RafaelGSS
fs: improve globSync performance
39af0aa 
PR-URL: #57725
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request May 1, 2025
2025-05-06, Version 24.0.0 (Current) #57609
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@Trott @RafaelGSS
fs: improve globSync performance
3edea66 
PR-URL: #57725
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@Trott @aduh95
fs: improve globSync performance
f6eadb5 
PR-URL: #57725
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@Trott @RafaelGSS
fs: improve globSync performance
721c171 
PR-URL: #57725
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
aduh95 pushed a commit that referenced this pull request May 16, 2025
@Trott @aduh95
fs: improve globSync performance
7246c69 
PR-URL: #57725
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@Trott @aduh95
fs: improve globSync performance
a6552c8 
PR-URL: #57725
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@github-actions github-actions Bot mentioned this pull request May 19, 2025
2025-05-21, Version 22.16.0 'Jod' (LTS) #58388
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
@Trott @aduh95
fs: improve globSync performance
8f64347 
PR-URL: #57725
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@marco-ippolito marco-ippolito added the dont-land-on-v20.x label Jun 5, 2025
@ghost ghost mentioned this pull request Jun 8, 2025
### Notable Changes autofix-ci/action#28
Closed
@Trott Trott mentioned this pull request Jul 24, 2025
fs: improve performance for fs.globSync() #59188
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig left review comments
@cjihrig cjihrig cjihrig approved these changes
@MoLow MoLow MoLow approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. fs Issues and PRs related to file-system APIs and the fs module. 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.

7 participants

@Trott @nodejs-github-bot @aduh95 @anonrig @cjihrig @MoLow @marco-ippolito
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.