Skip to content

fs: improve globSync performance - #57725

Merged
aduh95 merged 1 commit into
nodejs:mainfrom
Trott:glob-perf
Apr 7, 2025
Merged

fs: improve globSync performance#57725
aduh95 merged 1 commit into
nodejs:mainfrom
Trott:glob-perf

Conversation

@Trott

@TrottTrott 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 (***)

@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 Apr 2, 2025
@TrottTrott added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 2, 2025
@codecov

codecovBot commented Apr 2, 2025

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 linesCoverage Δ
lib/internal/fs/glob.js91.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.

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 2, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

// 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()) {

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.

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.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@aduh95
aduh95 merged commit b2ab1a2 into nodejs:mainApr 7, 2025
@aduh95

Copy link
Copy Markdown
Contributor

Landed in b2ab1a2

@Trott

Trott commented Apr 7, 2025

Copy link
Copy Markdown
MemberAuthor

I'll take a look at @anonrig's suggestion for a subsequent PR (unless someone beats me to it!).

@Trott
Trott deleted the glob-perf branch April 7, 2025 22:49
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #57725
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #57725
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57725
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
PR-URL: #57725
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57725
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57725
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 19, 2025
PR-URL: #57725
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@ghostghost mentioned this pull request Jun 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.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.

7 participants

@Trott@nodejs-github-bot@aduh95@anonrig@cjihrig@MoLow@marco-ippolito