Skip to content

fs: improve performance for fs.globSync() - #59188

Closed
Trott wants to merge 1 commit into
nodejs:mainfrom
Trott:glob-perf
Closed

fs: improve performance for fs.globSync()#59188
Trott wants to merge 1 commit into
nodejs:mainfrom
Trott:glob-perf

Conversation

@Trott

Copy link
Copy Markdown
Member

@nodejs-github-botnodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jul 23, 2025
@Trott
Trott marked this pull request as ready for review July 23, 2025 22:53
@Trott

Copy link
Copy Markdown
MemberAuthor

Letting GitHub Actions tests run before I set it back to draft mode and then run a benchmark job.

@codecov

codecovBot commented Jul 23, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.06%. Comparing base (58b5dc3) to head (310bc98).

Additional details and impacted files
@@ Coverage Diff @@## main #59188 +/- ##
=======================================
Coverage 90.05% 90.06% =======================================
Files 648 648 Lines 191025 191025 Branches 37451 37450 -1 =======================================
+ Hits 172032 172044 +12 - Misses 11605 11613 +8 + Partials 7388 7368 -20 
Files with missing linesCoverage Δ
lib/internal/fs/glob.js91.22% <100.00%> (-0.77%)⬇️

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

@Trott

Copy link
Copy Markdown
MemberAuthor

Oh no, looks like the benchmark compare job on CI no longer works?

@Trott

Copy link
Copy Markdown
MemberAuthor

I ran it locally and the numbers are not large or high-confidence. I'll run it a couple more times in different ways to see if it changes anything.

 confidence improvement accuracy (*) (**) (***)
fs/bench-glob.js recursive='false' mode='callback' pattern='*.js' dir='lib' n=1000 -0.15 % ±0.59% ±0.78% ±1.02%
fs/bench-glob.js recursive='false' mode='callback' pattern='**/*' dir='lib' n=1000 -0.12 % ±0.46% ±0.61% ±0.80%
fs/bench-glob.js recursive='false' mode='callback' pattern='**/**.js' dir='lib' n=1000 0.02 % ±0.44% ±0.58% ±0.76%
fs/bench-glob.js recursive='false' mode='promise' pattern='*.js' dir='lib' n=1000 -0.98 % ±2.98% ±3.97% ±5.18%
fs/bench-glob.js recursive='false' mode='promise' pattern='**/*' dir='lib' n=1000 0.83 % ±1.98% ±2.64% ±3.44%
fs/bench-glob.js recursive='false' mode='promise' pattern='**/**.js' dir='lib' n=1000 * 1.89 % ±1.74% ±2.32% ±3.04%
fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='lib' n=1000 -0.13 % ±0.80% ±1.06% ±1.38%
fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='lib' n=1000 0.15 % ±0.43% ±0.57% ±0.74%
fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='lib' n=1000 -0.02 % ±0.56% ±0.74% ±0.97%
fs/bench-glob.js recursive='true' mode='callback' pattern='*.js' dir='lib' n=1000 0.18 % ±0.49% ±0.65% ±0.85%
fs/bench-glob.js recursive='true' mode='callback' pattern='**/*' dir='lib' n=1000 0.21 % ±0.35% ±0.46% ±0.60%
fs/bench-glob.js recursive='true' mode='callback' pattern='**/**.js' dir='lib' n=1000 0.35 % ±0.42% ±0.56% ±0.73%
fs/bench-glob.js recursive='true' mode='promise' pattern='*.js' dir='lib' n=1000 -1.59 % ±2.20% ±2.93% ±3.82%
fs/bench-glob.js recursive='true' mode='promise' pattern='**/*' dir='lib' n=1000 -0.32 % ±2.04% ±2.72% ±3.55%
fs/bench-glob.js recursive='true' mode='promise' pattern='**/**.js' dir='lib' n=1000 3.61 % ±6.32% ±8.50% ±11.23%
fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='lib' n=1000 -0.04 % ±0.61% ±0.82% ±1.06%
fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='lib' n=1000 * 0.67 % ±0.56% ±0.75% ±0.98%
fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='lib' n=1000 0.12 % ±0.35% ±0.46% ±0.60%
Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 18 comparisons, you can thus expect the following amount of false-positive results:
0.90 false positives, when considering a 5% risk acceptance (*, **, ***),
0.18 false positives, when considering a 1% risk acceptance (**, ***),
0.02 false positives, when considering a 0.1% risk acceptance (***)

@Trott

Copy link
Copy Markdown
MemberAuthor

Subsequent benchmark run was not much different. @anonrig If you want to re-open this to check the benchmark code or improve the change here, go for it. But I think this can be closed.

@TrottTrott closed this Jul 24, 2025
@Trott

Copy link
Copy Markdown
MemberAuthor

I guess the way to get this thing's existence to show up in the other PR is to use #57725 (review) rather than the link I used in the PR description.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fsIssues and PRs related to the fs subsystem / file system.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Trott@nodejs-github-bot