Skip to content

benchmark: add callback-based fs.glob to glob benchmark #58417

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
nodejs-github-bot merged 3 commits into from
May 25, 2025
Merged

benchmark: add callback-based fs.glob to glob benchmark #58417

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65c4619
benchmark: add callback-based `fs.glob` to glob benchmark
LiviaMedeiros May 22, 2025
531d6be
squash: optimize benchmark
LiviaMedeiros May 22, 2025
9593907
squash: lint
LiviaMedeiros May 22, 2025
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
26 changes: 21 additions & 5 deletions benchmark/fs/bench-glob.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const configs = {
n: [1e3],
dir: ['lib'],
pattern: ['**/*', '*.js', '**/**.js'],
mode: ['async', 'sync'],
mode: ['sync', 'promise', 'callback'],
recursive: ['true', 'false'],
};

Expand All @@ -25,10 +25,26 @@ async function main(config) {
bench.start();

for (let i = 0; i < config.n; i++) {
if (mode === 'async') {
noDead = await fs.promises.glob(pattern, { cwd: fullPath, recursive });
} else {
noDead = fs.globSync(pattern, { cwd: fullPath, recursive });
switch (mode) {
case 'sync':
noDead = fs.globSync(pattern, { cwd: fullPath, recursive });
Copy link
Member

@H4ad H4ad May 22, 2025

Choose a reason for hiding this comment

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

I think we should build the options object outside the loop

Copy link
Member Author

@LiviaMedeiros LiviaMedeiros May 22, 2025

Choose a reason for hiding this comment

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

Sure, implemented in 531d6be

break;
case 'promise':
noDead = await fs.promises.glob(pattern, { cwd: fullPath, recursive });
break;
case 'callback':
noDead = await new Promise((resolve, reject) => {
fs.glob(pattern, { cwd: fullPath, recursive }, (err, matches) => {
if (err) {
reject(err);
} else {
resolve(matches);
}
});
Copy link
Member

@H4ad H4ad May 22, 2025

Choose a reason for hiding this comment

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

We can also move this callback method to outside the loop

Copy link
Member Author

@LiviaMedeiros LiviaMedeiros May 22, 2025

Choose a reason for hiding this comment

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

Implemented in 531d6be (whole function since we need the resolve and reject)

});
Copy link
Member

@H4ad H4ad May 22, 2025

Choose a reason for hiding this comment

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

Maybe add one version with util.promisify

Copy link
Member Author

@LiviaMedeiros LiviaMedeiros May 22, 2025

Choose a reason for hiding this comment

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

I don't really think so. It will make the benchmark run longer while giving no real benefit (we already measure the underlying callback-based function in this PR, and promisifying callback-based function makes little sense when promise-based function exists).

break;
default:
throw new Error(`Unknown mode: ${mode}`);
}
}

Expand Down
Loading