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
benchmark: add fs glob benchmark 
PR: #54644 (comment)
  • Loading branch information
@sonsurim
sonsurim committed Sep 9, 2024
commit f4025e5df16f928001466ad24127d52f120a158e
43 changes: 43 additions & 0 deletions benchmark/fs/bench-glob.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

const common = require('../common');
const fs = require('fs');
const path = require('path');

Comment thread
aduh95 marked this conversation as resolved.
const benchmarkDirectory = path.resolve(__dirname, '..', '..');

const configs = {
n: [1, 10, 100],
Copy link
Copy Markdown
Member

@avivkeller avivkeller Sep 12, 2024

Choose a reason for hiding this comment

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

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

100 is way to low.

dir: ['lib', 'test/parallel', 'benchmark'],
pattern: ['**/*', '*.js', '**/**.js'],
Copy link
Copy Markdown
Member

@daeyeon daeyeon Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@sonsurim sonsurim Sep 22, 2024

Choose a reason for hiding this comment

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

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'],

Copy link
Copy Markdown
Member

@daeyeon daeyeon Sep 22, 2024

Choose a reason for hiding this comment

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

Yes, I think the configs would be enough.

Copy link
Copy Markdown
Contributor Author

@sonsurim sonsurim Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

mode: ['async', 'sync'],
Comment thread
aduh95 marked this conversation as resolved.
recursive: ['true', 'false']
};

const bench = common.createBenchmark(main, configs);

async function main(conf) {
const fullPath = path.resolve(benchmarkDirectory, conf.dir);
const globPattern = conf.pattern;
const recursive = conf.recursive === 'true';

bench.start();
Copy link
Copy Markdown
Member

@avivkeller avivkeller Sep 12, 2024

Choose a reason for hiding this comment

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

Suggested change
bench.start();
let noDead;
bench.start();


let counter = 0;
Copy link
Copy Markdown
Member

@avivkeller avivkeller Sep 12, 2024

Choose a reason for hiding this comment

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

Suggested change
let counter = 0;

No need to count the output

for (let i = 0; i < conf.n; i++) {
if (conf.mode === 'async') {
const matches = await new Promise((resolve, reject) => {
fs.glob(globPattern, { cwd: fullPath, recursive }, (err, files) => {
if (err) reject(err);
else resolve(files);
});
});
counter += matches.length;
} else if (conf.mode === 'sync') {
const matches = fs.globSync(globPattern, { cwd: fullPath, recursive });
counter += matches.length;
}
}

bench.end(counter);
Copy link
Copy Markdown
Member

@avivkeller avivkeller Sep 12, 2024

Choose a reason for hiding this comment

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

Suggested change
bench.end(counter);
bench.end(config.n);

}