fs: refactoring declaratively with the Array.fromAsync function by sonsurim · Pull Request #54644 · nodejs/node

sonsurim · 2024-08-29T18:58:26Z

The following applies to this PR:

Replaced the for await...of loop with ArrayFromAsync().
Removed the need for manual array pushing using ArrayPrototypePush.
Added ArrayFromAsync type to primordials.d.ts file
Simplified the overall structure of the async function.

Array.fromAsync was defined relatively recently, but I used it because I knew it was officially adopted in Node.js v22. (MDN Document)

- Refs: nodejs#51912

codecov · 2024-08-29T20:48:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.24%. Comparing base (e70bd47) to head (defe492).
Report is 476 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54644      +/-   ##
==========================================
+ Coverage   87.34%   88.24%   +0.90%     
==========================================
  Files         649      651       +2     
  Lines      182524   183875    +1351     
  Branches    35026    35862     +836     
==========================================
+ Hits       159420   162264    +2844     
+ Misses      16373    14896    -1477     
+ Partials     6731     6715      -16

Files with missing lines	Coverage Δ
lib/fs.js	`98.14% <100.00%> (+4.88%)`	⬆️

... and 237 files with indirect coverage changes

avivkeller · 2024-08-29T20:56:16Z

lib/fs.js

-      for await (const entry of new Glob(pattern, options).glob()) {
-        ArrayPrototypePush(res, entry);
-      }
+      const res = await ArrayFromAsync(new Glob(pattern, options).glob());


I'm not sure if there is a benchmark for this, but this should probably be benchmarked.

@redyetidev Thanks for the review!
There are currently no benchmarks, so I'll add them!

@redyetidev
Sorry for being late!
I added the fs glob benchmark f4025e5.

The benchmark results are as follows:

node-benchmark-compare compare-glob-result.csv confidence improvement accuracy (*) (**) (***) fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='benchmark' n=1 0.68 % ±1.50% ±2.01% ±2.62% fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='benchmark' n=10 0.47 % ±1.59% ±2.11% ±2.75% fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='benchmark' n=100 * 0.68 % ±0.63% ±0.84% ±1.10% fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='lib' n=1 -0.40 % ±1.61% ±2.15% ±2.82% fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='lib' n=10 0.23 % ±1.31% ±1.74% ±2.27% fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='lib' n=100 1.67 % ±1.67% ±2.24% ±2.96% fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='test/parallel' n=1 0.77 % ±0.83% ±1.11% ±1.46% fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='test/parallel' n=10 *** 3.75 % ±1.90% ±2.55% ±3.37% fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='test/parallel' n=100 * 1.51 % ±1.16% ±1.54% ±2.01% fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='benchmark' n=1 0.13 % ±1.53% ±2.04% ±2.67% fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='benchmark' n=10 1.42 % ±1.64% ±2.20% ±2.91% fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='benchmark' n=100 0.27 % ±0.53% ±0.71% ±0.93% fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='lib' n=1 *** 1.39 % ±0.75% ±1.00% ±1.31% fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='lib' n=10 -3.22 % ±5.11% ±6.88% ±9.13% fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='lib' n=100 0.05 % ±0.78% ±1.04% ±1.35% fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='test/parallel' n=1 1.09 % ±1.37% ±1.82% ±2.38% fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='test/parallel' n=10 *** 5.67 % ±1.26% ±1.69% ±2.23% fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='test/parallel' n=100 1.22 % ±1.55% ±2.07% ±2.72% fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='benchmark' n=1 0.39 % ±3.01% ±4.03% ±5.27% fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='benchmark' n=10 0.72 % ±1.31% ±1.74% ±2.27% fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='benchmark' n=100 0.35 % ±1.11% ±1.49% ±1.97% fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='lib' n=1 1.28 % ±1.48% ±1.99% ±2.62% fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='lib' n=10 0.33 % ±1.02% ±1.35% ±1.76% fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='lib' n=100 0.03 % ±0.71% ±0.94% ±1.22% fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='test/parallel' n=1 * 1.49 % ±1.44% ±1.92% ±2.51% fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='test/parallel' n=10 *** 7.41 % ±2.81% ±3.79% ±5.03% fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='test/parallel' n=100 *** 1.56 % ±0.75% ±1.00% ±1.31% fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='benchmark' n=1 -0.43 % ±1.26% ±1.68% ±2.19% fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='benchmark' n=10 -0.12 % ±1.12% ±1.49% ±1.94% fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='benchmark' n=100 -0.10 % ±0.91% ±1.21% ±1.58% fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='lib' n=1 2.79 % ±4.15% ±5.59% ±7.39% fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='lib' n=10 0.12 % ±2.12% ±2.83% ±3.70% fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='lib' n=100 0.07 % ±3.03% ±4.04% ±5.26% fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='test/parallel' n=1 0.41 % ±1.25% ±1.66% ±2.17% fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='test/parallel' n=10 0.79 % ±2.11% ±2.81% ±3.66% fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='test/parallel' n=100 0.08 % ±0.92% ±1.22% ±1.59% fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='benchmark' n=1 -1.47 % ±4.43% ±5.96% ±7.90% fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='benchmark' n=10 0.49 % ±0.51% ±0.68% ±0.89% fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='benchmark' n=100 1.15 % ±1.23% ±1.66% ±2.19% fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='lib' n=1 0.57 % ±0.91% ±1.21% ±1.58% fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='lib' n=10 -1.87 % ±4.44% ±5.97% ±7.90% fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='lib' n=100 2.01 % ±3.92% ±5.29% ±7.01% fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='test/parallel' n=1 -0.10 % ±0.88% ±1.18% ±1.54% fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='test/parallel' n=10 -0.12 % ±0.83% ±1.11% ±1.45% fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='test/parallel' n=100 -0.17 % ±0.79% ±1.06% ±1.38% fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='benchmark' n=1 * 1.12 % ±1.08% ±1.44% ±1.88% fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='benchmark' n=10 1.85 % ±2.73% ±3.65% ±4.80% fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='benchmark' n=100 0.16 % ±1.07% ±1.42% ±1.85% fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='lib' n=1 0.54 % ±1.06% ±1.42% ±1.84% fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='lib' n=10 -0.62 % ±1.21% ±1.62% ±2.14% fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='lib' n=100 0.66 % ±1.14% ±1.52% ±2.00% fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='test/parallel' n=1 -2.67 % ±5.08% ±6.85% ±9.09% fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='test/parallel' n=10 -0.12 % ±0.73% ±0.97% ±1.26% fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='test/parallel' n=100 0.55 % ±0.60% ±0.80% ±1.05% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='benchmark' n=1 0.22 % ±1.42% ±1.90% ±2.49% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='benchmark' n=10 0.08 % ±2.60% ±3.50% ±4.64% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='benchmark' n=100 0.08 % ±0.78% ±1.03% ±1.35% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='lib' n=1 -0.03 % ±1.26% ±1.68% ±2.20% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='lib' n=10 0.38 % ±2.30% ±3.07% ±4.03% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='lib' n=100 2.81 % ±3.46% ±4.65% ±6.16% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='test/parallel' n=1 *** 1.77 % ±0.50% ±0.66% ±0.86% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='test/parallel' n=10 *** 4.75 % ±1.45% ±1.93% ±2.52% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='test/parallel' n=100 0.26 % ±1.28% ±1.72% ±2.27% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='benchmark' n=1 0.45 % ±1.22% ±1.63% ±2.13% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='benchmark' n=10 0.31 % ±0.81% ±1.09% ±1.42% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='benchmark' n=100 -0.18 % ±0.84% ±1.13% ±1.47% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='lib' n=1 0.21 % ±1.48% ±1.97% ±2.56% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='lib' n=10 -0.23 % ±0.57% ±0.76% ±1.00% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='lib' n=100 0.05 % ±1.43% ±1.92% ±2.52% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='test/parallel' n=1 ** 2.56 % ±1.63% ±2.18% ±2.88% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='test/parallel' n=10 *** 4.31 % ±1.28% ±1.71% ±2.23% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='test/parallel' n=100 * 1.00 % ±0.76% ±1.02% ±1.34% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='benchmark' n=1 2.96 % ±4.42% ±5.95% ±7.89% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='benchmark' n=10 0.75 % ±1.01% ±1.35% ±1.78% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='benchmark' n=100 0.04 % ±0.75% ±1.00% ±1.32% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='lib' n=1 1.02 % ±1.31% ±1.74% ±2.27% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='lib' n=10 -0.29 % ±1.13% ±1.52% ±1.99% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='lib' n=100 0.26 % ±0.70% ±0.93% ±1.21% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='test/parallel' n=1 *** 2.64 % ±1.26% ±1.69% ±2.23% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='test/parallel' n=10 *** 5.59 % ±1.19% ±1.59% ±2.10% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='test/parallel' n=100 ** 1.59 % ±1.12% ±1.49% ±1.96% fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='benchmark' n=1 1.62 % ±2.70% ±3.61% ±4.72% fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='benchmark' n=10 -0.09 % ±1.25% ±1.67% ±2.20% fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='benchmark' n=100 0.03 % ±0.60% ±0.80% ±1.04% fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='lib' n=1 0.87 % ±1.34% ±1.78% ±2.32% fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='lib' n=10 0.81 % ±1.74% ±2.32% ±3.03% fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='lib' n=100 0.71 % ±1.92% ±2.56% ±3.35% fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='test/parallel' n=1 -0.14 % ±0.75% ±1.01% ±1.33% fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='test/parallel' n=10 0.16 % ±0.61% ±0.81% ±1.06% fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='test/parallel' n=100 0.09 % ±1.20% ±1.60% ±2.08% fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='benchmark' n=1 0.35 % ±1.17% ±1.57% ±2.06% fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='benchmark' n=10 0.77 % ±0.79% ±1.05% ±1.38% fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='benchmark' n=100 1.09 % ±1.19% ±1.60% ±2.12% fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='lib' n=1 0.57 % ±1.04% ±1.38% ±1.80% fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='lib' n=10 -0.57 % ±2.63% ±3.52% ±4.65% fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='lib' n=100 2.33 % ±3.84% ±5.17% ±6.86% fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='test/parallel' n=1 -0.26 % ±0.60% ±0.80% ±1.04% fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='test/parallel' n=10 0.28 % ±0.66% ±0.88% ±1.15% fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='test/parallel' n=100 0.05 % ±0.93% ±1.24% ±1.61% fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='benchmark' n=1 * 1.61 % ±1.44% ±1.94% ±2.55% fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='benchmark' n=10 0.77 % ±3.60% ±4.82% ±6.33% fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='benchmark' n=100 1.17 % ±1.52% ±2.05% ±2.71% fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='lib' n=1 0.88 % ±0.93% ±1.23% ±1.60% fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='lib' n=10 -0.21 % ±0.92% ±1.22% ±1.59% fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='lib' n=100 0.86 % ±1.28% ±1.72% ±2.27% fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='test/parallel' n=1 0.28 % ±0.50% ±0.66% ±0.87% fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='test/parallel' n=10 1.55 % ±2.77% ±3.71% ±4.90% fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='test/parallel' n=100 * 0.86 % ±0.86% ±1.15% ±1.52% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case, there are 108 comparisons, you can thus expect the following amount of false-positive results: 5.40 false positives, when considering a 5% risk acceptance (*, **, ***), 1.08 false positives, when considering a 1% risk acceptance (**, ***), 0.11 false positives, when considering a 0.1% risk acceptance (***)

Performance is significantly improved when asynchronous compared to synchronous.

@redyetidev
Please confirm when you have time. Many Thanks!

I don't think that benchmark is accurate. See my review comments.

@redyetidev
Many Thanks, we were able to improve the benchmark code.
Here are the results of the improved benchmark!

The results of recursive=true useAsync=true are:

confidence improvement accuracy (*) (**) (***) fs/bench-glob.js recursive='true' useAsync='true' pattern='*.js' dir='benchmark' n=1000 1.03 % ±2.95% ±3.95% ±5.21% fs/bench-glob.js recursive='true' useAsync='true' pattern='*.js' dir='lib' n=1000 1.14 % ±1.67% ±2.23% ±2.90% fs/bench-glob.js recursive='true' useAsync='true' pattern='*.js' dir='test/parallel' n=1000 * 1.53 % ±1.32% ±1.75% ±2.28% fs/bench-glob.js recursive='true' useAsync='true' pattern='**/*' dir='benchmark' n=1000 0.70 % ±1.27% ±1.70% ±2.23% fs/bench-glob.js recursive='true' useAsync='true' pattern='**/*' dir='lib' n=1000 0.97 % ±2.06% ±2.75% ±3.58% fs/bench-glob.js recursive='true' useAsync='true' pattern='**/*' dir='test/parallel' n=1000 0.73 % ±1.55% ±2.07% ±2.69% fs/bench-glob.js recursive='true' useAsync='true' pattern='**/**.js' dir='benchmark' n=1000 -0.88 % ±1.31% ±1.74% ±2.27% fs/bench-glob.js recursive='true' useAsync='true' pattern='**/**.js' dir='lib' n=1000 1.23 % ±2.51% ±3.35% ±4.36% fs/bench-glob.js recursive='true' useAsync='true' pattern='**/**.js' dir='test/parallel' n=1000 0.66 % ±1.42% ±1.89% ±2.47% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case, there are 9 comparisons, you can thus expect the following amount of false-positive results: 0.45 false positives, when considering a 5% risk acceptance (*, **, ***), 0.09 false positives, when considering a 1% risk acceptance (**, ***), 0.01 false positives, when considering a 0.1% risk acceptance (***)

The results of recursive=true useAsync=false are:

confidence improvement accuracy (*) (**) (***) fs/bench-glob.js recursive='true' useAsync='false' pattern='*.js' dir='benchmark' n=1000 0.33 % ±2.25% ±3.00% ±3.92% fs/bench-glob.js recursive='true' useAsync='false' pattern='*.js' dir='lib' n=1000 2.97 % ±4.11% ±5.52% ±7.27% fs/bench-glob.js recursive='true' useAsync='false' pattern='*.js' dir='test/parallel' n=1000 1.08 % ±1.16% ±1.54% ±2.01% fs/bench-glob.js recursive='true' useAsync='false' pattern='**/*' dir='benchmark' n=1000 0.59 % ±1.82% ±2.43% ±3.16% fs/bench-glob.js recursive='true' useAsync='false' pattern='**/*' dir='lib' n=1000 0.40 % ±1.85% ±2.47% ±3.22% fs/bench-glob.js recursive='true' useAsync='false' pattern='**/*' dir='test/parallel' n=1000 1.25 % ±1.52% ±2.02% ±2.63% fs/bench-glob.js recursive='true' useAsync='false' pattern='**/**.js' dir='benchmark' n=1000 0.33 % ±1.56% ±2.08% ±2.71% fs/bench-glob.js recursive='true' useAsync='false' pattern='**/**.js' dir='lib' n=1000 0.42 % ±1.68% ±2.24% ±2.91% fs/bench-glob.js recursive='true' useAsync='false' pattern='**/**.js' dir='test/parallel' n=1000 1.03 % ±1.15% ±1.53% ±1.99% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case, there are 9 comparisons, you can thus expect the following amount of false-positive results: 0.45 false positives, when considering a 5% risk acceptance (*, **, ***), 0.09 false positives, when considering a 1% risk acceptance (**, ***), 0.01 false positives, when considering a 0.1% risk acceptance (***)

Nice! Looks like a bit of improvement

PR: nodejs#54644 (comment)

avivkeller · 2024-09-12T10:40:27Z

@nodejs/fs @nodejs/benchmarking

avivkeller · 2024-09-12T13:22:28Z

benchmark/fs/bench-glob.js

+  dir: ['lib', 'test/parallel', 'benchmark'],
+  pattern: ['**/*', '*.js', '**/**.js'],
+  mode: ['async', 'sync'],
+  recursive: ['true', 'false'],


Suggested change

recursive: ['true', 'false'],

recursive: [true, false],

I didn't seem to use boolean in benchmark (error on CI), so I fixed that in this commit!

Many Thanks!

benchmark/fs/bench-glob.js

avivkeller · 2024-09-12T13:23:36Z

benchmark/fs/bench-glob.js

+const benchmarkDirectory = path.resolve(__dirname, '..', '..');
+
+const configs = {
+  n: [1, 10, 100],


IMO n should be a single (higher) value. Something like 3e4 (but that might be too high)?

100 is way to low.

avivkeller · 2024-09-12T13:25:10Z

benchmark/fs/bench-glob.js

+
+  bench.start();
+
+  let counter = 0;


Suggested change

let counter = 0;

No need to count the output

avivkeller · 2024-09-12T13:25:17Z

benchmark/fs/bench-glob.js

+    }
+  }
+
+  bench.end(counter);


Suggested change

bench.end(counter);

bench.end(config.n);

avivkeller · 2024-09-12T13:25:35Z

lib/fs.js

-      for await (const entry of new Glob(pattern, options).glob()) {
-        ArrayPrototypePush(res, entry);
-      }
+      const res = await ArrayFromAsync(new Glob(pattern, options).glob());


I don't think that benchmark is accurate. See my review comments.

avivkeller · 2024-09-12T15:52:24Z

benchmark/fs/bench-glob.js

+  const fullPath = path.resolve(benchmarkDirectory, config.dir);
+  const { pattern, recursive, useAsync } = config;
+
+  bench.start();


Suggested change

bench.start();

let noDead;

bench.start();

avivkeller · 2024-09-12T15:52:34Z

benchmark/fs/bench-glob.js

+
+  for (let i = 0; i < config.n; i++) {
+    if (useAsync) {
+      await new Promise((resolve, reject) => {


Suggested change

await new Promise((resolve, reject) => {

noDead = await new Promise((resolve, reject) => {

avivkeller · 2024-09-12T15:52:47Z

benchmark/fs/bench-glob.js

+        });
+      });
+    } else {
+      fs.globSync(pattern, { cwd: fullPath, recursive });


Suggested change

fs.globSync(pattern, { cwd: fullPath, recursive });

noDead = fs.globSync(pattern, { cwd: fullPath, recursive });

avivkeller · 2024-09-12T15:53:02Z

benchmark/fs/bench-glob.js

+    }
+  }
+
+  bench.end(config.n);


Suggested change

bench.end(config.n);

bench.end(config.n);

assert.ok(noDead);

benchmark/fs/bench-glob.js

sonsurim · 2024-09-12T15:58:37Z

@redyetidev
It was my first benchmarking, so I was clumsy. Thank you so much for the review!

modify config property because boolean cannot be used

avivkeller · 2024-09-13T23:55:39Z

benchmark/fs/bench-glob.js

+  bench.start();
+
+  for (let i = 0; i < config.n; i++) {
+    if (mode === 'async') {


Does this statement affect benchmarking speeds?

@redyetidev
Thank you for the review!
I think wrapping a Promise with each iteration affects benchmarking, So I fixed it.
After benchmarking again, there seems to be a decrease in performance..🥲

So, I'm trying to close the current PR and upload a PR that only adds benchmarking code. Is that okay?

mode: async, recursive: true

confidence improvement accuracy (*) (**) (***) fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='benchmark' n=1000 -0.55 % ±1.61% ±2.14% ±2.79% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='lib' n=1000 -0.16 % ±1.88% ±2.51% ±3.27% fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='test/parallel' n=1000 1.76 % ±3.88% ±5.19% ±6.83% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='benchmark' n=1000 ** -3.39 % ±2.20% ±2.93% ±3.83% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='lib' n=1000 -0.69 % ±2.17% ±2.89% ±3.77% fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='test/parallel' n=1000 -0.20 % ±1.24% ±1.65% ±2.14% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='benchmark' n=1000 -2.56 % ±2.61% ±3.49% ±4.57% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='lib' n=1000 -0.49 % ±2.24% ±2.98% ±3.87% fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='test/parallel' n=1000 -1.24 % ±1.48% ±1.97% ±2.56% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case, there are 9 comparisons, you can thus expect the following amount of false-positive results: 0.45 false positives, when considering a 5% risk acceptance (*, **, ***), 0.09 false positives, when considering a 1% risk acceptance (**, ***), 0.01 false positives, when considering a 0.1% risk acceptance (***)

@redyetidev, Sorry for the frequent comments.
I look forward to hearing from you.
Many thanks!

nodejs-github-bot · 2024-09-21T05:23:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62617/

daeyeon · 2024-09-21T10:36:22Z

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1642/

Details

16:22:11                                                                                               confidence improvement accuracy (*)   (**)  (***)
16:22:11 fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='benchmark' n=1000                         1.68 %       ±3.31% ±4.41% ±5.74%
16:22:11 fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='lib' n=1000                               0.30 %       ±3.80% ±5.06% ±6.58%
16:22:11 fs/bench-glob.js recursive='false' mode='async' pattern='*.js' dir='test/parallel' n=1000                     0.37 %       ±2.93% ±3.90% ±5.07%
16:22:11 fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='benchmark' n=1000                        -0.82 %       ±3.44% ±4.58% ±5.97%
16:22:11 fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='lib' n=1000                               1.91 %       ±3.57% ±4.76% ±6.20%
16:22:11 fs/bench-glob.js recursive='false' mode='async' pattern='**/*' dir='test/parallel' n=1000                     2.36 %       ±3.06% ±4.07% ±5.29%
16:22:11 fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='benchmark' n=1000                     1.26 %       ±3.21% ±4.28% ±5.58%
16:22:11 fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='lib' n=1000                          -0.32 %       ±2.79% ±3.71% ±4.83%
16:22:11 fs/bench-glob.js recursive='false' mode='async' pattern='**/**.js' dir='test/parallel' n=1000                 0.01 %       ±3.08% ±4.09% ±5.33%
16:22:11 fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='benchmark' n=1000                         -0.19 %       ±0.58% ±0.77% ±1.01%
16:22:11 fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='lib' n=1000                               -0.25 %       ±0.63% ±0.83% ±1.09%
16:22:11 fs/bench-glob.js recursive='false' mode='sync' pattern='*.js' dir='test/parallel' n=1000                      0.01 %       ±0.25% ±0.33% ±0.43%
16:22:11 fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='benchmark' n=1000                         -0.06 %       ±0.22% ±0.30% ±0.39%
16:22:11 fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='lib' n=1000                                0.10 %       ±0.26% ±0.34% ±0.45%
16:22:11 fs/bench-glob.js recursive='false' mode='sync' pattern='**/*' dir='test/parallel' n=1000                     -0.10 %       ±0.14% ±0.18% ±0.24%
16:22:11 fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='benchmark' n=1000                     -0.06 %       ±0.36% ±0.47% ±0.62%
16:22:11 fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='lib' n=1000                            0.08 %       ±0.27% ±0.36% ±0.47%
16:22:11 fs/bench-glob.js recursive='false' mode='sync' pattern='**/**.js' dir='test/parallel' n=1000                  0.07 %       ±0.25% ±0.33% ±0.43%
16:22:11 fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='benchmark' n=1000                         -1.57 %       ±4.24% ±5.65% ±7.36%
16:22:11 fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='lib' n=1000                                1.11 %       ±4.70% ±6.26% ±8.16%
16:22:11 fs/bench-glob.js recursive='true' mode='async' pattern='*.js' dir='test/parallel' n=1000                      1.93 %       ±4.49% ±5.98% ±7.78%
16:22:11 fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='benchmark' n=1000                          3.49 %       ±4.63% ±6.17% ±8.03%
16:22:11 fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='lib' n=1000                                0.54 %       ±3.81% ±5.08% ±6.62%
16:22:11 fs/bench-glob.js recursive='true' mode='async' pattern='**/*' dir='test/parallel' n=1000                     -1.94 %       ±4.00% ±5.32% ±6.93%
16:22:11 fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='benchmark' n=1000                     -1.03 %       ±3.26% ±4.35% ±5.67%
16:22:11 fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='lib' n=1000                            0.21 %       ±3.82% ±5.08% ±6.62%
16:22:11 fs/bench-glob.js recursive='true' mode='async' pattern='**/**.js' dir='test/parallel' n=1000                  1.81 %       ±4.73% ±6.29% ±8.19%
16:22:11 fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='benchmark' n=1000                           0.26 %       ±0.65% ±0.87% ±1.13%
16:22:11 fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='lib' n=1000                                 0.52 %       ±0.80% ±1.07% ±1.39%
16:22:11 fs/bench-glob.js recursive='true' mode='sync' pattern='*.js' dir='test/parallel' n=1000                      -0.04 %       ±0.27% ±0.36% ±0.47%
16:22:11 fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='benchmark' n=1000                           0.12 %       ±0.22% ±0.29% ±0.38%
16:22:11 fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='lib' n=1000                                 0.12 %       ±0.20% ±0.27% ±0.36%
16:22:11 fs/bench-glob.js recursive='true' mode='sync' pattern='**/*' dir='test/parallel' n=1000                       0.21 %       ±0.25% ±0.33% ±0.43%
16:22:11 fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='benchmark' n=1000                *      0.19 %       ±0.17% ±0.23% ±0.30%
16:22:11 fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='lib' n=1000                            -0.02 %       ±0.22% ±0.29% ±0.37%
16:22:11 fs/bench-glob.js recursive='true' mode='sync' pattern='**/**.js' dir='test/parallel' n=1000                   0.11 %       ±0.26% ±0.34% ±0.45%

daeyeon · 2024-09-21T10:53:29Z

benchmark/fs/bench-glob.js

+  dir: ['lib', 'test/parallel', 'benchmark'],
+  pattern: ['**/*', '*.js', '**/**.js'],


nit: There seem to be too many cases. Suggest reducing a few options if possible. Please consider if it's possible to obtain sufficient results with fewer iterations and cases.

Thank you so much for the review!
If it's the review you mentioned, can I ask if it would be a good idea to try reducing the number of dir and pattern types?

I thought about it this way:

dir: ['lib'], pattern: ['**/*', '*.js', '**/**.js'],

Yes, I think the configs would be enough.

Thank you! There were definitely a variety of cases..! I fixed from defe492!

nodejs-github-bot · 2024-09-24T23:35:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62755/

nodejs-github-bot · 2024-09-25T03:36:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62756/

aduh95 · 2024-09-27T09:59:12Z

Landed in 090add7

Refs: nodejs#51912 PR-URL: nodejs#54644 Reviewed-By: Daeyeon Jeong <[email protected]>

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Aug 29, 2024

fs: refactoring declaratively with the fromAsync function

07d075a

- Refs: nodejs#51912

sonsurim force-pushed the refactor-glob branch from 4dd3a9e to 07d075a Compare August 29, 2024 19:03

sonsurim added 2 commits August 30, 2024 04:15

fs: replace global Array to primordials ArrayFromAsync

e2d8a69

fix lint error

dde434c

avivkeller reviewed Aug 29, 2024

View reviewed changes

sonsurim added 2 commits September 9, 2024 17:16

benchmark: add fs glob benchmark

f4025e5

PR: nodejs#54644 (comment)

lint js

1a9ae15

avivkeller added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 12, 2024

avivkeller added the benchmark Issues and PRs related to the benchmark subsystem. label Sep 12, 2024

refactor glob benchmark

db16e20

avivkeller reviewed Sep 12, 2024

View reviewed changes

avivkeller added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 12, 2024

sonsurim added 2 commits September 12, 2024 23:30

reflected review in glob benchmark

43ef1e4

remove an unused variable

cb2591a

avivkeller reviewed Sep 12, 2024

View reviewed changes

applied review on glob benchmark

1ff400e

fix error where benchmarking type does not match in CI

c976446

modify config property because boolean cannot be used

sonsurim force-pushed the refactor-glob branch from ed1d2a8 to c976446 Compare September 13, 2024 22:06

avivkeller reviewed Sep 13, 2024

View reviewed changes

remove parts that affect benchmarking

7dbb93f

daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024

daeyeon reviewed Sep 21, 2024

View reviewed changes

daeyeon approved these changes Sep 21, 2024

View reviewed changes

fix too many cases for benchmarking

defe492

daeyeon 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 Sep 24, 2024

daeyeon approved these changes Sep 24, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 27, 2024

aduh95 merged commit 090add7 into nodejs:main Sep 27, 2024

sonsurim deleted the refactor-glob branch September 27, 2024 14:05

targos added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Oct 4, 2024

aduh95 mentioned this pull request Oct 14, 2024

2024-10-16, Version 23.0.0 (Current) #55338

Merged

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024

fs: refactoring declaratively with Array.fromAsync

055c0ae

Refs: nodejs#51912 PR-URL: nodejs#54644 Reviewed-By: Daeyeon Jeong <[email protected]>

tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024

fs: refactoring declaratively with Array.fromAsync

bb9e84a

Refs: nodejs#51912 PR-URL: nodejs#54644 Reviewed-By: Daeyeon Jeong <[email protected]>

	await new Promise((resolve, reject) => {
	noDead = await new Promise((resolve, reject) => {

	fs.globSync(pattern, { cwd: fullPath, recursive });
	noDead = fs.globSync(pattern, { cwd: fullPath, recursive });

		dir: ['lib', 'test/parallel', 'benchmark'],
		pattern: ['*/', '.js', '/*.js'],

Uh oh!

Conversation

sonsurim commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonsurim Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avivkeller commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonsurim Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonsurim commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 21, 2024

Uh oh!

daeyeon commented Sep 21, 2024

Uh oh!

daeyeon Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonsurim Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 24, 2024

Uh oh!

nodejs-github-bot commented Sep 25, 2024

sonsurim commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

sonsurim Sep 12, 2024 •

edited

Loading

avivkeller commented Sep 12, 2024 •

edited

Loading

sonsurim Sep 13, 2024 •

edited

Loading

sonsurim commented Sep 12, 2024 •

edited

Loading

daeyeon Sep 21, 2024 •

edited

Loading

sonsurim Sep 24, 2024 •

edited

Loading