Skip to content

fs: refactoring declaratively with the Array.fromAsync function #54644

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduh95 merged 12 commits into from
Sep 27, 2024
Merged

fs: refactoring declaratively with the Array.fromAsync function #54644

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
07d075a
fs: refactoring declaratively with the fromAsync function
sonsurim Aug 29, 2024
e2d8a69
fs: replace global Array to primordials ArrayFromAsync
sonsurim Aug 29, 2024
dde434c
fix lint error
sonsurim Aug 29, 2024
f4025e5
benchmark: add fs glob benchmark
sonsurim Sep 9, 2024
1a9ae15
lint js
sonsurim Sep 9, 2024
db16e20
refactor glob benchmark
sonsurim Sep 12, 2024
43ef1e4
reflected review in glob benchmark
sonsurim Sep 12, 2024
cb2591a
remove an unused variable
sonsurim Sep 12, 2024
1ff400e
applied review on glob benchmark
sonsurim Sep 12, 2024
c976446
fix error where benchmarking type does not match in CI
sonsurim Sep 13, 2024
7dbb93f
remove parts that affect benchmarking
sonsurim Sep 14, 2024
defe492
fix too many cases for benchmarking
sonsurim Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
remove parts that affect benchmarking
  • Loading branch information
@sonsurim
sonsurim committed Sep 14, 2024
commit 7dbb93fcde5ff6b2113943f2c9f0b138359db4ea
10 changes: 1 addition & 9 deletions benchmark/fs/bench-glob.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,7 @@ async function main(config) {

for (let i = 0; i < config.n; i++) {
if (mode === 'async') {
Copy link
Copy Markdown
Member

@avivkeller avivkeller Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this statement affect benchmarking speeds?

Copy link
Copy Markdown
Contributor Author

@sonsurim sonsurim Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@sonsurim sonsurim Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

noDead = await new Promise((resolve, reject) => {
fs.glob(pattern, { cwd: fullPath, recursive }, (err, files) => {
if (err) {
reject(err);
} else {
resolve(files);
};
});
});
noDead = await fs.promises.glob(pattern, { cwd: fullPath, recursive });
} else {
noDead = fs.globSync(pattern, { cwd: fullPath, recursive });
}
Expand Down