fs: improve performance of recursive directory read - #65524

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf
Closed

fs: improve performance of recursive directory read#65524
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf

Conversation

@avivkeller

@avivkelleravivkeller commented Aug 24, 2026

Copy link
Copy Markdown
Member

Currently, when recursively reading a directory, we do a few things which can be considered slow:

  1. We round-trip between CPP and JS for each entry, meaning that a 5.4k-entry large directory needs to round-trip 5.4k times, after this PR, that's only one round trip.
  2. We stat'd every file previously, and now, we only stat when needed, using the native lstat over the JS version.

Benchmarks:

 confidence improvement accuracy (*) (**) (***)
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='lib' n=10 *** 248.34 % ±22.14% ±29.62% ±38.87%
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='test/parallel' n=10 *** 471.49 % ±45.94% ±61.85% ±81.99%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='lib' n=10 *** 243.07 % ±25.02% ±33.53% ±44.11%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='test/parallel' n=10 *** 485.55 % ±37.61% ±50.61% ±67.05%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='lib' n=10 *** 188.53 % ±21.79% ±29.19% ±38.39%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='test/parallel' n=10 *** 497.94 % ±31.68% ±42.61% ±56.39%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='lib' n=10 *** 118.38 % ±21.14% ±28.27% ±37.08%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='test/parallel' n=10 *** 290.39 % ±21.57% ±28.89% ±37.99%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='lib' n=10 *** 53.59 % ±14.66% ±19.53% ±25.47%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='test/parallel' n=10 8.72 % ±9.27% ±12.33% ±16.06%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='lib' n=10 *** 71.40 % ±14.63% ±19.52% ±25.51%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='test/parallel' n=10 *** 269.65 % ±26.21% ±35.19% ±46.45%
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 (***)

Also,
Fixes#58892

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 24, 2026
@avivkelleravivkeller added the fs Issues and PRs related to file-system APIs and the fs module. label Aug 24, 2026
@avivkeller

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/fs

@codecov

codecovBot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.71429% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (05a8e91) to head (7538782).
⚠️ Report is 107 commits behind head on main.

Files with missing linesPatch %Lines
src/node_file.cc79.37%27 Missing and 26 partials ⚠️
lib/internal/fs/utils.js69.87%25 Missing ⚠️
lib/internal/fs/promises.js36.36%21 Missing ⚠️
lib/fs.js93.61%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #65524 +/- ##
==========================================
- Coverage 90.06% 90.02% -0.05% 
==========================================
Files 751 754 +3 Lines 254919 256705 +1786 Branches 48124 48552 +428 ==========================================
+ Hits 229603 231099 +1496 - Misses 16492 16701 +209 - Partials 8824 8905 +81 
Files with missing linesCoverage Δ
lib/fs.js97.34% <93.61%> (-1.10%)⬇️
lib/internal/fs/promises.js91.32% <36.36%> (-0.98%)⬇️
lib/internal/fs/utils.js96.04% <69.87%> (-1.88%)⬇️
src/node_file.cc74.51% <79.37%> (+0.38%)⬆️

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

@avivkeller
avivkeller marked this pull request as draft August 25, 2026 03:53
@avivkeller
avivkeller marked this pull request as ready for review August 25, 2026 03:56

@codebyterecodebytere 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.

thanks for picking this up! fyi the table in the description is against main before #65487 landed, which already dropped the per-entry stat() and went to one binding call per directory, so most of that delta is gone. against current main (linux x64, means of 5 runs of this PR's benchmark, plus dir=test for a nested tree with 860 dirs / 13k entries) i get:

mainthis PR
promise, lib362 ops/s895 (+147 %)
promise, test25.059.9 (+140 %)
sync, lib8501561 (+84 %)
sync, test57.565.7 (+14 %)
sync + withFileTypes, lib930801 (−14 %)
sync + withFileTypes, test55.456.8 (n.s.)
any mode, test/parallel (flat)n.s.

so the promises path is the clear win (it awaited one thread pool round trip per directory), sync is a modest win, and withFileTypes sync regresses on small trees, which i think is the marshalling (comment below). could you rerun against current main and update the description please?

Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadlib/internal/fs/utils.js Outdated
Comment threadlib/internal/fs/promises.js
Comment threadbenchmark/fs/bench-readdir-recursive.js
Comment threadsrc/node_file.cc Outdated
@avivkelleravivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@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

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Fixes: nodejs#58892
Refs: nodejs#52663
Signed-off-by: avivkeller <me@aviv.sh>

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

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

Copy link
Copy Markdown
Collaborator

avivkeller added a commit that referenced this pull request Sep 2, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Landed in 927dfad

aduh95 pushed a commit that referenced this pull request Sep 3, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to file-system APIs and the fs module.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

all versions of readdir don't work in recursive mode when used with a buffer argument

6 participants

@avivkeller@nodejs-github-bot@mcollina@anonrig@codebytere@gurgunday
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fs: improve performance of recursive directory read - #65524

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf
Closed

fs: improve performance of recursive directory read#65524
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf

Conversation

@avivkeller

@avivkelleravivkeller commented Aug 24, 2026

Copy link
Copy Markdown
Member

Currently, when recursively reading a directory, we do a few things which can be considered slow:

  1. We round-trip between CPP and JS for each entry, meaning that a 5.4k-entry large directory needs to round-trip 5.4k times, after this PR, that's only one round trip.
  2. We stat'd every file previously, and now, we only stat when needed, using the native lstat over the JS version.

Benchmarks:

 confidence improvement accuracy (*) (**) (***)
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='lib' n=10 *** 248.34 % ±22.14% ±29.62% ±38.87%
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='test/parallel' n=10 *** 471.49 % ±45.94% ±61.85% ±81.99%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='lib' n=10 *** 243.07 % ±25.02% ±33.53% ±44.11%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='test/parallel' n=10 *** 485.55 % ±37.61% ±50.61% ±67.05%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='lib' n=10 *** 188.53 % ±21.79% ±29.19% ±38.39%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='test/parallel' n=10 *** 497.94 % ±31.68% ±42.61% ±56.39%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='lib' n=10 *** 118.38 % ±21.14% ±28.27% ±37.08%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='test/parallel' n=10 *** 290.39 % ±21.57% ±28.89% ±37.99%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='lib' n=10 *** 53.59 % ±14.66% ±19.53% ±25.47%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='test/parallel' n=10 8.72 % ±9.27% ±12.33% ±16.06%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='lib' n=10 *** 71.40 % ±14.63% ±19.52% ±25.51%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='test/parallel' n=10 *** 269.65 % ±26.21% ±35.19% ±46.45%
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 (***)

Also,
Fixes#58892

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 24, 2026
@avivkelleravivkeller added the fs Issues and PRs related to file-system APIs and the fs module. label Aug 24, 2026
@avivkeller

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/fs

@codecov

codecovBot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.71429% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (05a8e91) to head (7538782).
⚠️ Report is 107 commits behind head on main.

Files with missing linesPatch %Lines
src/node_file.cc79.37%27 Missing and 26 partials ⚠️
lib/internal/fs/utils.js69.87%25 Missing ⚠️
lib/internal/fs/promises.js36.36%21 Missing ⚠️
lib/fs.js93.61%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #65524 +/- ##
==========================================
- Coverage 90.06% 90.02% -0.05% 
==========================================
Files 751 754 +3 Lines 254919 256705 +1786 Branches 48124 48552 +428 ==========================================
+ Hits 229603 231099 +1496 - Misses 16492 16701 +209 - Partials 8824 8905 +81 
Files with missing linesCoverage Δ
lib/fs.js97.34% <93.61%> (-1.10%)⬇️
lib/internal/fs/promises.js91.32% <36.36%> (-0.98%)⬇️
lib/internal/fs/utils.js96.04% <69.87%> (-1.88%)⬇️
src/node_file.cc74.51% <79.37%> (+0.38%)⬆️

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

@avivkeller
avivkeller marked this pull request as draft August 25, 2026 03:53
@avivkeller
avivkeller marked this pull request as ready for review August 25, 2026 03:56

@codebyterecodebytere 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.

thanks for picking this up! fyi the table in the description is against main before #65487 landed, which already dropped the per-entry stat() and went to one binding call per directory, so most of that delta is gone. against current main (linux x64, means of 5 runs of this PR's benchmark, plus dir=test for a nested tree with 860 dirs / 13k entries) i get:

mainthis PR
promise, lib362 ops/s895 (+147 %)
promise, test25.059.9 (+140 %)
sync, lib8501561 (+84 %)
sync, test57.565.7 (+14 %)
sync + withFileTypes, lib930801 (−14 %)
sync + withFileTypes, test55.456.8 (n.s.)
any mode, test/parallel (flat)n.s.

so the promises path is the clear win (it awaited one thread pool round trip per directory), sync is a modest win, and withFileTypes sync regresses on small trees, which i think is the marshalling (comment below). could you rerun against current main and update the description please?

Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadlib/internal/fs/utils.js Outdated
Comment threadlib/internal/fs/promises.js
Comment threadbenchmark/fs/bench-readdir-recursive.js
Comment threadsrc/node_file.cc Outdated
@avivkelleravivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@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

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Fixes: nodejs#58892
Refs: nodejs#52663
Signed-off-by: avivkeller <me@aviv.sh>

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

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

Copy link
Copy Markdown
Collaborator

avivkeller added a commit that referenced this pull request Sep 2, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Landed in 927dfad

aduh95 pushed a commit that referenced this pull request Sep 3, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to file-system APIs and the fs module.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

all versions of readdir don't work in recursive mode when used with a buffer argument

6 participants

@avivkeller@nodejs-github-bot@mcollina@anonrig@codebytere@gurgunday
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fs: improve performance of recursive directory read - #65524

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf
Closed

fs: improve performance of recursive directory read#65524
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf

Conversation

@avivkeller

@avivkelleravivkeller commented Aug 24, 2026

Copy link
Copy Markdown
Member

Currently, when recursively reading a directory, we do a few things which can be considered slow:

  1. We round-trip between CPP and JS for each entry, meaning that a 5.4k-entry large directory needs to round-trip 5.4k times, after this PR, that's only one round trip.
  2. We stat'd every file previously, and now, we only stat when needed, using the native lstat over the JS version.

Benchmarks:

 confidence improvement accuracy (*) (**) (***)
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='lib' n=10 *** 248.34 % ±22.14% ±29.62% ±38.87%
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='test/parallel' n=10 *** 471.49 % ±45.94% ±61.85% ±81.99%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='lib' n=10 *** 243.07 % ±25.02% ±33.53% ±44.11%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='test/parallel' n=10 *** 485.55 % ±37.61% ±50.61% ±67.05%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='lib' n=10 *** 188.53 % ±21.79% ±29.19% ±38.39%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='test/parallel' n=10 *** 497.94 % ±31.68% ±42.61% ±56.39%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='lib' n=10 *** 118.38 % ±21.14% ±28.27% ±37.08%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='test/parallel' n=10 *** 290.39 % ±21.57% ±28.89% ±37.99%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='lib' n=10 *** 53.59 % ±14.66% ±19.53% ±25.47%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='test/parallel' n=10 8.72 % ±9.27% ±12.33% ±16.06%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='lib' n=10 *** 71.40 % ±14.63% ±19.52% ±25.51%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='test/parallel' n=10 *** 269.65 % ±26.21% ±35.19% ±46.45%
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 (***)

Also,
Fixes#58892

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 24, 2026
@avivkelleravivkeller added the fs Issues and PRs related to file-system APIs and the fs module. label Aug 24, 2026
@avivkeller

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/fs

@codecov

codecovBot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.71429% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (05a8e91) to head (7538782).
⚠️ Report is 107 commits behind head on main.

Files with missing linesPatch %Lines
src/node_file.cc79.37%27 Missing and 26 partials ⚠️
lib/internal/fs/utils.js69.87%25 Missing ⚠️
lib/internal/fs/promises.js36.36%21 Missing ⚠️
lib/fs.js93.61%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #65524 +/- ##
==========================================
- Coverage 90.06% 90.02% -0.05% 
==========================================
Files 751 754 +3 Lines 254919 256705 +1786 Branches 48124 48552 +428 ==========================================
+ Hits 229603 231099 +1496 - Misses 16492 16701 +209 - Partials 8824 8905 +81 
Files with missing linesCoverage Δ
lib/fs.js97.34% <93.61%> (-1.10%)⬇️
lib/internal/fs/promises.js91.32% <36.36%> (-0.98%)⬇️
lib/internal/fs/utils.js96.04% <69.87%> (-1.88%)⬇️
src/node_file.cc74.51% <79.37%> (+0.38%)⬆️

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

@avivkeller
avivkeller marked this pull request as draft August 25, 2026 03:53
@avivkeller
avivkeller marked this pull request as ready for review August 25, 2026 03:56

@codebyterecodebytere 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.

thanks for picking this up! fyi the table in the description is against main before #65487 landed, which already dropped the per-entry stat() and went to one binding call per directory, so most of that delta is gone. against current main (linux x64, means of 5 runs of this PR's benchmark, plus dir=test for a nested tree with 860 dirs / 13k entries) i get:

mainthis PR
promise, lib362 ops/s895 (+147 %)
promise, test25.059.9 (+140 %)
sync, lib8501561 (+84 %)
sync, test57.565.7 (+14 %)
sync + withFileTypes, lib930801 (−14 %)
sync + withFileTypes, test55.456.8 (n.s.)
any mode, test/parallel (flat)n.s.

so the promises path is the clear win (it awaited one thread pool round trip per directory), sync is a modest win, and withFileTypes sync regresses on small trees, which i think is the marshalling (comment below). could you rerun against current main and update the description please?

Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadlib/internal/fs/utils.js Outdated
Comment threadlib/internal/fs/promises.js
Comment threadbenchmark/fs/bench-readdir-recursive.js
Comment threadsrc/node_file.cc Outdated
@avivkelleravivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@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

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Fixes: nodejs#58892
Refs: nodejs#52663
Signed-off-by: avivkeller <me@aviv.sh>

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

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

Copy link
Copy Markdown
Collaborator

avivkeller added a commit that referenced this pull request Sep 2, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Landed in 927dfad

aduh95 pushed a commit that referenced this pull request Sep 3, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to file-system APIs and the fs module.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

all versions of readdir don't work in recursive mode when used with a buffer argument

6 participants

@avivkeller@nodejs-github-bot@mcollina@anonrig@codebytere@gurgunday
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fs: improve performance of recursive directory read - #65524

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf
Closed

fs: improve performance of recursive directory read#65524
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf

Conversation

@avivkeller

@avivkelleravivkeller commented Aug 24, 2026

Copy link
Copy Markdown
Member

Currently, when recursively reading a directory, we do a few things which can be considered slow:

  1. We round-trip between CPP and JS for each entry, meaning that a 5.4k-entry large directory needs to round-trip 5.4k times, after this PR, that's only one round trip.
  2. We stat'd every file previously, and now, we only stat when needed, using the native lstat over the JS version.

Benchmarks:

 confidence improvement accuracy (*) (**) (***)
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='lib' n=10 *** 248.34 % ±22.14% ±29.62% ±38.87%
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='test/parallel' n=10 *** 471.49 % ±45.94% ±61.85% ±81.99%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='lib' n=10 *** 243.07 % ±25.02% ±33.53% ±44.11%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='test/parallel' n=10 *** 485.55 % ±37.61% ±50.61% ±67.05%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='lib' n=10 *** 188.53 % ±21.79% ±29.19% ±38.39%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='test/parallel' n=10 *** 497.94 % ±31.68% ±42.61% ±56.39%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='lib' n=10 *** 118.38 % ±21.14% ±28.27% ±37.08%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='test/parallel' n=10 *** 290.39 % ±21.57% ±28.89% ±37.99%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='lib' n=10 *** 53.59 % ±14.66% ±19.53% ±25.47%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='test/parallel' n=10 8.72 % ±9.27% ±12.33% ±16.06%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='lib' n=10 *** 71.40 % ±14.63% ±19.52% ±25.51%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='test/parallel' n=10 *** 269.65 % ±26.21% ±35.19% ±46.45%
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 (***)

Also,
Fixes#58892

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 24, 2026
@avivkelleravivkeller added the fs Issues and PRs related to file-system APIs and the fs module. label Aug 24, 2026
@avivkeller

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/fs

@codecov

codecovBot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.71429% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (05a8e91) to head (7538782).
⚠️ Report is 107 commits behind head on main.

Files with missing linesPatch %Lines
src/node_file.cc79.37%27 Missing and 26 partials ⚠️
lib/internal/fs/utils.js69.87%25 Missing ⚠️
lib/internal/fs/promises.js36.36%21 Missing ⚠️
lib/fs.js93.61%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #65524 +/- ##
==========================================
- Coverage 90.06% 90.02% -0.05% 
==========================================
Files 751 754 +3 Lines 254919 256705 +1786 Branches 48124 48552 +428 ==========================================
+ Hits 229603 231099 +1496 - Misses 16492 16701 +209 - Partials 8824 8905 +81 
Files with missing linesCoverage Δ
lib/fs.js97.34% <93.61%> (-1.10%)⬇️
lib/internal/fs/promises.js91.32% <36.36%> (-0.98%)⬇️
lib/internal/fs/utils.js96.04% <69.87%> (-1.88%)⬇️
src/node_file.cc74.51% <79.37%> (+0.38%)⬆️

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

@avivkeller
avivkeller marked this pull request as draft August 25, 2026 03:53
@avivkeller
avivkeller marked this pull request as ready for review August 25, 2026 03:56

@codebyterecodebytere 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.

thanks for picking this up! fyi the table in the description is against main before #65487 landed, which already dropped the per-entry stat() and went to one binding call per directory, so most of that delta is gone. against current main (linux x64, means of 5 runs of this PR's benchmark, plus dir=test for a nested tree with 860 dirs / 13k entries) i get:

mainthis PR
promise, lib362 ops/s895 (+147 %)
promise, test25.059.9 (+140 %)
sync, lib8501561 (+84 %)
sync, test57.565.7 (+14 %)
sync + withFileTypes, lib930801 (−14 %)
sync + withFileTypes, test55.456.8 (n.s.)
any mode, test/parallel (flat)n.s.

so the promises path is the clear win (it awaited one thread pool round trip per directory), sync is a modest win, and withFileTypes sync regresses on small trees, which i think is the marshalling (comment below). could you rerun against current main and update the description please?

Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadlib/internal/fs/utils.js Outdated
Comment threadlib/internal/fs/promises.js
Comment threadbenchmark/fs/bench-readdir-recursive.js
Comment threadsrc/node_file.cc Outdated
@avivkelleravivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@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

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Fixes: nodejs#58892
Refs: nodejs#52663
Signed-off-by: avivkeller <me@aviv.sh>

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

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

Copy link
Copy Markdown
Collaborator

avivkeller added a commit that referenced this pull request Sep 2, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Landed in 927dfad

aduh95 pushed a commit that referenced this pull request Sep 3, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to file-system APIs and the fs module.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

all versions of readdir don't work in recursive mode when used with a buffer argument

6 participants

@avivkeller@nodejs-github-bot@mcollina@anonrig@codebytere@gurgunday
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fs: improve performance of recursive directory read - #65524

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf
Closed

fs: improve performance of recursive directory read#65524
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf

Conversation

@avivkeller

@avivkelleravivkeller commented Aug 24, 2026

Copy link
Copy Markdown
Member

Currently, when recursively reading a directory, we do a few things which can be considered slow:

  1. We round-trip between CPP and JS for each entry, meaning that a 5.4k-entry large directory needs to round-trip 5.4k times, after this PR, that's only one round trip.
  2. We stat'd every file previously, and now, we only stat when needed, using the native lstat over the JS version.

Benchmarks:

 confidence improvement accuracy (*) (**) (***)
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='lib' n=10 *** 248.34 % ±22.14% ±29.62% ±38.87%
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='test/parallel' n=10 *** 471.49 % ±45.94% ±61.85% ±81.99%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='lib' n=10 *** 243.07 % ±25.02% ±33.53% ±44.11%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='test/parallel' n=10 *** 485.55 % ±37.61% ±50.61% ±67.05%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='lib' n=10 *** 188.53 % ±21.79% ±29.19% ±38.39%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='test/parallel' n=10 *** 497.94 % ±31.68% ±42.61% ±56.39%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='lib' n=10 *** 118.38 % ±21.14% ±28.27% ±37.08%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='test/parallel' n=10 *** 290.39 % ±21.57% ±28.89% ±37.99%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='lib' n=10 *** 53.59 % ±14.66% ±19.53% ±25.47%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='test/parallel' n=10 8.72 % ±9.27% ±12.33% ±16.06%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='lib' n=10 *** 71.40 % ±14.63% ±19.52% ±25.51%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='test/parallel' n=10 *** 269.65 % ±26.21% ±35.19% ±46.45%
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 (***)

Also,
Fixes#58892

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 24, 2026
@avivkelleravivkeller added the fs Issues and PRs related to file-system APIs and the fs module. label Aug 24, 2026
@avivkeller

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/fs

@codecov

codecovBot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.71429% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (05a8e91) to head (7538782).
⚠️ Report is 107 commits behind head on main.

Files with missing linesPatch %Lines
src/node_file.cc79.37%27 Missing and 26 partials ⚠️
lib/internal/fs/utils.js69.87%25 Missing ⚠️
lib/internal/fs/promises.js36.36%21 Missing ⚠️
lib/fs.js93.61%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #65524 +/- ##
==========================================
- Coverage 90.06% 90.02% -0.05% 
==========================================
Files 751 754 +3 Lines 254919 256705 +1786 Branches 48124 48552 +428 ==========================================
+ Hits 229603 231099 +1496 - Misses 16492 16701 +209 - Partials 8824 8905 +81 
Files with missing linesCoverage Δ
lib/fs.js97.34% <93.61%> (-1.10%)⬇️
lib/internal/fs/promises.js91.32% <36.36%> (-0.98%)⬇️
lib/internal/fs/utils.js96.04% <69.87%> (-1.88%)⬇️
src/node_file.cc74.51% <79.37%> (+0.38%)⬆️

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

@avivkeller
avivkeller marked this pull request as draft August 25, 2026 03:53
@avivkeller
avivkeller marked this pull request as ready for review August 25, 2026 03:56

@codebyterecodebytere 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.

thanks for picking this up! fyi the table in the description is against main before #65487 landed, which already dropped the per-entry stat() and went to one binding call per directory, so most of that delta is gone. against current main (linux x64, means of 5 runs of this PR's benchmark, plus dir=test for a nested tree with 860 dirs / 13k entries) i get:

mainthis PR
promise, lib362 ops/s895 (+147 %)
promise, test25.059.9 (+140 %)
sync, lib8501561 (+84 %)
sync, test57.565.7 (+14 %)
sync + withFileTypes, lib930801 (−14 %)
sync + withFileTypes, test55.456.8 (n.s.)
any mode, test/parallel (flat)n.s.

so the promises path is the clear win (it awaited one thread pool round trip per directory), sync is a modest win, and withFileTypes sync regresses on small trees, which i think is the marshalling (comment below). could you rerun against current main and update the description please?

Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadlib/internal/fs/utils.js Outdated
Comment threadlib/internal/fs/promises.js
Comment threadbenchmark/fs/bench-readdir-recursive.js
Comment threadsrc/node_file.cc Outdated
@avivkelleravivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@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

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Fixes: nodejs#58892
Refs: nodejs#52663
Signed-off-by: avivkeller <me@aviv.sh>

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

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

Copy link
Copy Markdown
Collaborator

avivkeller added a commit that referenced this pull request Sep 2, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Landed in 927dfad

aduh95 pushed a commit that referenced this pull request Sep 3, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to file-system APIs and the fs module.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

all versions of readdir don't work in recursive mode when used with a buffer argument

6 participants

@avivkeller@nodejs-github-bot@mcollina@anonrig@codebytere@gurgunday
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fs: improve performance of recursive directory read - #65524

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf
Closed

fs: improve performance of recursive directory read#65524
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf

Conversation

@avivkeller

@avivkelleravivkeller commented Aug 24, 2026

Copy link
Copy Markdown
Member

Currently, when recursively reading a directory, we do a few things which can be considered slow:

  1. We round-trip between CPP and JS for each entry, meaning that a 5.4k-entry large directory needs to round-trip 5.4k times, after this PR, that's only one round trip.
  2. We stat'd every file previously, and now, we only stat when needed, using the native lstat over the JS version.

Benchmarks:

 confidence improvement accuracy (*) (**) (***)
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='lib' n=10 *** 248.34 % ±22.14% ±29.62% ±38.87%
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='test/parallel' n=10 *** 471.49 % ±45.94% ±61.85% ±81.99%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='lib' n=10 *** 243.07 % ±25.02% ±33.53% ±44.11%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='test/parallel' n=10 *** 485.55 % ±37.61% ±50.61% ±67.05%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='lib' n=10 *** 188.53 % ±21.79% ±29.19% ±38.39%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='test/parallel' n=10 *** 497.94 % ±31.68% ±42.61% ±56.39%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='lib' n=10 *** 118.38 % ±21.14% ±28.27% ±37.08%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='test/parallel' n=10 *** 290.39 % ±21.57% ±28.89% ±37.99%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='lib' n=10 *** 53.59 % ±14.66% ±19.53% ±25.47%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='test/parallel' n=10 8.72 % ±9.27% ±12.33% ±16.06%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='lib' n=10 *** 71.40 % ±14.63% ±19.52% ±25.51%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='test/parallel' n=10 *** 269.65 % ±26.21% ±35.19% ±46.45%
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 (***)

Also,
Fixes#58892

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 24, 2026
@avivkelleravivkeller added the fs Issues and PRs related to file-system APIs and the fs module. label Aug 24, 2026
@avivkeller

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/fs

@codecov

codecovBot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.71429% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (05a8e91) to head (7538782).
⚠️ Report is 107 commits behind head on main.

Files with missing linesPatch %Lines
src/node_file.cc79.37%27 Missing and 26 partials ⚠️
lib/internal/fs/utils.js69.87%25 Missing ⚠️
lib/internal/fs/promises.js36.36%21 Missing ⚠️
lib/fs.js93.61%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #65524 +/- ##
==========================================
- Coverage 90.06% 90.02% -0.05% 
==========================================
Files 751 754 +3 Lines 254919 256705 +1786 Branches 48124 48552 +428 ==========================================
+ Hits 229603 231099 +1496 - Misses 16492 16701 +209 - Partials 8824 8905 +81 
Files with missing linesCoverage Δ
lib/fs.js97.34% <93.61%> (-1.10%)⬇️
lib/internal/fs/promises.js91.32% <36.36%> (-0.98%)⬇️
lib/internal/fs/utils.js96.04% <69.87%> (-1.88%)⬇️
src/node_file.cc74.51% <79.37%> (+0.38%)⬆️

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

@avivkeller
avivkeller marked this pull request as draft August 25, 2026 03:53
@avivkeller
avivkeller marked this pull request as ready for review August 25, 2026 03:56

@codebyterecodebytere 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.

thanks for picking this up! fyi the table in the description is against main before #65487 landed, which already dropped the per-entry stat() and went to one binding call per directory, so most of that delta is gone. against current main (linux x64, means of 5 runs of this PR's benchmark, plus dir=test for a nested tree with 860 dirs / 13k entries) i get:

mainthis PR
promise, lib362 ops/s895 (+147 %)
promise, test25.059.9 (+140 %)
sync, lib8501561 (+84 %)
sync, test57.565.7 (+14 %)
sync + withFileTypes, lib930801 (−14 %)
sync + withFileTypes, test55.456.8 (n.s.)
any mode, test/parallel (flat)n.s.

so the promises path is the clear win (it awaited one thread pool round trip per directory), sync is a modest win, and withFileTypes sync regresses on small trees, which i think is the marshalling (comment below). could you rerun against current main and update the description please?

Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadlib/internal/fs/utils.js Outdated
Comment threadlib/internal/fs/promises.js
Comment threadbenchmark/fs/bench-readdir-recursive.js
Comment threadsrc/node_file.cc Outdated
@avivkelleravivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@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

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Fixes: nodejs#58892
Refs: nodejs#52663
Signed-off-by: avivkeller <me@aviv.sh>

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

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

Copy link
Copy Markdown
Collaborator

avivkeller added a commit that referenced this pull request Sep 2, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Landed in 927dfad

aduh95 pushed a commit that referenced this pull request Sep 3, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to file-system APIs and the fs module.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

all versions of readdir don't work in recursive mode when used with a buffer argument

6 participants

@avivkeller@nodejs-github-bot@mcollina@anonrig@codebytere@gurgunday
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fs: improve performance of recursive directory read - #65524

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf
Closed

fs: improve performance of recursive directory read#65524
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf

Conversation

@avivkeller

@avivkelleravivkeller commented Aug 24, 2026

Copy link
Copy Markdown
Member

Currently, when recursively reading a directory, we do a few things which can be considered slow:

  1. We round-trip between CPP and JS for each entry, meaning that a 5.4k-entry large directory needs to round-trip 5.4k times, after this PR, that's only one round trip.
  2. We stat'd every file previously, and now, we only stat when needed, using the native lstat over the JS version.

Benchmarks:

 confidence improvement accuracy (*) (**) (***)
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='lib' n=10 *** 248.34 % ±22.14% ±29.62% ±38.87%
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='test/parallel' n=10 *** 471.49 % ±45.94% ±61.85% ±81.99%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='lib' n=10 *** 243.07 % ±25.02% ±33.53% ±44.11%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='test/parallel' n=10 *** 485.55 % ±37.61% ±50.61% ±67.05%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='lib' n=10 *** 188.53 % ±21.79% ±29.19% ±38.39%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='test/parallel' n=10 *** 497.94 % ±31.68% ±42.61% ±56.39%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='lib' n=10 *** 118.38 % ±21.14% ±28.27% ±37.08%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='test/parallel' n=10 *** 290.39 % ±21.57% ±28.89% ±37.99%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='lib' n=10 *** 53.59 % ±14.66% ±19.53% ±25.47%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='test/parallel' n=10 8.72 % ±9.27% ±12.33% ±16.06%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='lib' n=10 *** 71.40 % ±14.63% ±19.52% ±25.51%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='test/parallel' n=10 *** 269.65 % ±26.21% ±35.19% ±46.45%
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 (***)

Also,
Fixes#58892

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 24, 2026
@avivkelleravivkeller added the fs Issues and PRs related to file-system APIs and the fs module. label Aug 24, 2026
@avivkeller

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/fs

@codecov

codecovBot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.71429% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (05a8e91) to head (7538782).
⚠️ Report is 107 commits behind head on main.

Files with missing linesPatch %Lines
src/node_file.cc79.37%27 Missing and 26 partials ⚠️
lib/internal/fs/utils.js69.87%25 Missing ⚠️
lib/internal/fs/promises.js36.36%21 Missing ⚠️
lib/fs.js93.61%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #65524 +/- ##
==========================================
- Coverage 90.06% 90.02% -0.05% 
==========================================
Files 751 754 +3 Lines 254919 256705 +1786 Branches 48124 48552 +428 ==========================================
+ Hits 229603 231099 +1496 - Misses 16492 16701 +209 - Partials 8824 8905 +81 
Files with missing linesCoverage Δ
lib/fs.js97.34% <93.61%> (-1.10%)⬇️
lib/internal/fs/promises.js91.32% <36.36%> (-0.98%)⬇️
lib/internal/fs/utils.js96.04% <69.87%> (-1.88%)⬇️
src/node_file.cc74.51% <79.37%> (+0.38%)⬆️

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

@avivkeller
avivkeller marked this pull request as draft August 25, 2026 03:53
@avivkeller
avivkeller marked this pull request as ready for review August 25, 2026 03:56

@codebyterecodebytere 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.

thanks for picking this up! fyi the table in the description is against main before #65487 landed, which already dropped the per-entry stat() and went to one binding call per directory, so most of that delta is gone. against current main (linux x64, means of 5 runs of this PR's benchmark, plus dir=test for a nested tree with 860 dirs / 13k entries) i get:

mainthis PR
promise, lib362 ops/s895 (+147 %)
promise, test25.059.9 (+140 %)
sync, lib8501561 (+84 %)
sync, test57.565.7 (+14 %)
sync + withFileTypes, lib930801 (−14 %)
sync + withFileTypes, test55.456.8 (n.s.)
any mode, test/parallel (flat)n.s.

so the promises path is the clear win (it awaited one thread pool round trip per directory), sync is a modest win, and withFileTypes sync regresses on small trees, which i think is the marshalling (comment below). could you rerun against current main and update the description please?

Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadlib/internal/fs/utils.js Outdated
Comment threadlib/internal/fs/promises.js
Comment threadbenchmark/fs/bench-readdir-recursive.js
Comment threadsrc/node_file.cc Outdated
@avivkelleravivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@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

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Fixes: nodejs#58892
Refs: nodejs#52663
Signed-off-by: avivkeller <me@aviv.sh>

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

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

Copy link
Copy Markdown
Collaborator

avivkeller added a commit that referenced this pull request Sep 2, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Landed in 927dfad

aduh95 pushed a commit that referenced this pull request Sep 3, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to file-system APIs and the fs module.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

all versions of readdir don't work in recursive mode when used with a buffer argument

6 participants

@avivkeller@nodejs-github-bot@mcollina@anonrig@codebytere@gurgunday
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fs: improve performance of recursive directory read - #65524

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf
Closed

fs: improve performance of recursive directory read#65524
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:fs-perf

Conversation

@avivkeller

@avivkelleravivkeller commented Aug 24, 2026

Copy link
Copy Markdown
Member

Currently, when recursively reading a directory, we do a few things which can be considered slow:

  1. We round-trip between CPP and JS for each entry, meaning that a 5.4k-entry large directory needs to round-trip 5.4k times, after this PR, that's only one round trip.
  2. We stat'd every file previously, and now, we only stat when needed, using the native lstat over the JS version.

Benchmarks:

 confidence improvement accuracy (*) (**) (***)
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='lib' n=10 *** 248.34 % ±22.14% ±29.62% ±38.87%
fs/bench-readdir-recursive.js withFileTypes='false' mode='callback' dir='test/parallel' n=10 *** 471.49 % ±45.94% ±61.85% ±81.99%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='lib' n=10 *** 243.07 % ±25.02% ±33.53% ±44.11%
fs/bench-readdir-recursive.js withFileTypes='false' mode='promise' dir='test/parallel' n=10 *** 485.55 % ±37.61% ±50.61% ±67.05%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='lib' n=10 *** 188.53 % ±21.79% ±29.19% ±38.39%
fs/bench-readdir-recursive.js withFileTypes='false' mode='sync' dir='test/parallel' n=10 *** 497.94 % ±31.68% ±42.61% ±56.39%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='lib' n=10 *** 118.38 % ±21.14% ±28.27% ±37.08%
fs/bench-readdir-recursive.js withFileTypes='true' mode='callback' dir='test/parallel' n=10 *** 290.39 % ±21.57% ±28.89% ±37.99%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='lib' n=10 *** 53.59 % ±14.66% ±19.53% ±25.47%
fs/bench-readdir-recursive.js withFileTypes='true' mode='promise' dir='test/parallel' n=10 8.72 % ±9.27% ±12.33% ±16.06%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='lib' n=10 *** 71.40 % ±14.63% ±19.52% ±25.51%
fs/bench-readdir-recursive.js withFileTypes='true' mode='sync' dir='test/parallel' n=10 *** 269.65 % ±26.21% ±35.19% ±46.45%
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 (***)

Also,
Fixes#58892

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 24, 2026
@avivkelleravivkeller added the fs Issues and PRs related to file-system APIs and the fs module. label Aug 24, 2026
@avivkeller

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/fs

@codecov

codecovBot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.71429% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (05a8e91) to head (7538782).
⚠️ Report is 107 commits behind head on main.

Files with missing linesPatch %Lines
src/node_file.cc79.37%27 Missing and 26 partials ⚠️
lib/internal/fs/utils.js69.87%25 Missing ⚠️
lib/internal/fs/promises.js36.36%21 Missing ⚠️
lib/fs.js93.61%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #65524 +/- ##
==========================================
- Coverage 90.06% 90.02% -0.05% 
==========================================
Files 751 754 +3 Lines 254919 256705 +1786 Branches 48124 48552 +428 ==========================================
+ Hits 229603 231099 +1496 - Misses 16492 16701 +209 - Partials 8824 8905 +81 
Files with missing linesCoverage Δ
lib/fs.js97.34% <93.61%> (-1.10%)⬇️
lib/internal/fs/promises.js91.32% <36.36%> (-0.98%)⬇️
lib/internal/fs/utils.js96.04% <69.87%> (-1.88%)⬇️
src/node_file.cc74.51% <79.37%> (+0.38%)⬆️

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

@avivkeller
avivkeller marked this pull request as draft August 25, 2026 03:53
@avivkeller
avivkeller marked this pull request as ready for review August 25, 2026 03:56

@codebyterecodebytere 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.

thanks for picking this up! fyi the table in the description is against main before #65487 landed, which already dropped the per-entry stat() and went to one binding call per directory, so most of that delta is gone. against current main (linux x64, means of 5 runs of this PR's benchmark, plus dir=test for a nested tree with 860 dirs / 13k entries) i get:

mainthis PR
promise, lib362 ops/s895 (+147 %)
promise, test25.059.9 (+140 %)
sync, lib8501561 (+84 %)
sync, test57.565.7 (+14 %)
sync + withFileTypes, lib930801 (−14 %)
sync + withFileTypes, test55.456.8 (n.s.)
any mode, test/parallel (flat)n.s.

so the promises path is the clear win (it awaited one thread pool round trip per directory), sync is a modest win, and withFileTypes sync regresses on small trees, which i think is the marshalling (comment below). could you rerun against current main and update the description please?

Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadlib/internal/fs/utils.js Outdated
Comment threadlib/internal/fs/promises.js
Comment threadbenchmark/fs/bench-readdir-recursive.js
Comment threadsrc/node_file.cc Outdated
@avivkelleravivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@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

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Fixes: nodejs#58892
Refs: nodejs#52663
Signed-off-by: avivkeller <me@aviv.sh>

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

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

Copy link
Copy Markdown
Collaborator

avivkeller added a commit that referenced this pull request Sep 2, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Landed in 927dfad

aduh95 pushed a commit that referenced this pull request Sep 3, 2026
Fixes: #58892
Refs: #52663
Signed-off-by: avivkeller <me@aviv.sh>
PR-URL: #65524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to file-system APIs and the fs module.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

all versions of readdir don't work in recursive mode when used with a buffer argument

6 participants

@avivkeller@nodejs-github-bot@mcollina@anonrig@codebytere@gurgunday