Skip to content

fs: Performance Enhancement: Concurrent Execution of Async Tasks with Promise.all #51215

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.

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

sanjaiyan-dev
Copy link
Contributor

Hello,

This pull request introduces a performance improvement by executing asynchronous tasks concurrently using Promise.all. This enhancement aims to optimize the utilization of hardware resources effectively.

This PR draws inspiration from a similar one (though not identical) in the Deno runtime, where the execution time was reduced by 85% through concurrent processing. For reference, please see: denoland/std#3363.

Additionally, a comparable pull request has been made in the Deno JavaScript runtime, as demonstrated in: denoland/std#3683.

Thank you for considering these enhancements.

@nodejs-github-bot 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 Dec 19, 2023
@sanjaiyan-dev sanjaiyan-dev changed the title Performance Enhancement: Concurrent Execution of Async Tasks with Promise.all fs: Performance Enhancement: Concurrent Execution of Async Tasks with Promise.all Dec 19, 2023
@sanjaiyan-dev sanjaiyan-dev requested a review from aduh95 December 20, 2023 04:38
@sanjaiyan-dev
Copy link
Contributor Author

Sorry @aduh95, I didn't understand what the PromisePrototypePush function does. Please explain. Sorry for disturbing you.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

You need to import ArrayPrototypsePush and SafePromiseAllReturnVoid from the primordials object at the top of the file.

Primordials wrap the original prototype functions with new functions that take
the `this` value as the first argument:
```js
const {
ArrayPrototypePush,
} = primordials;
const array = [1, 2, 3];
ArrayPrototypePush(array, 4);
console.log(JSON.stringify(array)); // [1,2,3,4]
Array.prototype.push = function push(val) {
return this.unshift(val);
};
ArrayPrototypePush(array, 5);
console.log(JSON.stringify(array)); // [1,2,3,4,5]
```

@sanjaiyan-dev
Copy link
Contributor Author

You need to import ArrayPrototypsePush and SafePromiseAllReturnVoid from the primordials object at the top of the file.

Primordials wrap the original prototype functions with new functions that take
the `this` value as the first argument:
```js
const {
ArrayPrototypePush,
} = primordials;
const array = [1, 2, 3];
ArrayPrototypePush(array, 4);
console.log(JSON.stringify(array)); // [1,2,3,4]
Array.prototype.push = function push(val) {
return this.unshift(val);
};
ArrayPrototypePush(array, 5);
console.log(JSON.stringify(array)); // [1,2,3,4,5]
```

Thanks for the explanation. I will fix it ASAP.

@sanjaiyan-dev sanjaiyan-dev requested a review from aduh95 December 20, 2023 17:39
@sanjaiyan-dev
Copy link
Contributor Author

I hope the current code changes are correct. Apologies for any inconvenience. (@aduh95 )

@sanjaiyan-dev sanjaiyan-dev requested a review from MoLow December 24, 2023 19:02
@@ -159,10 +160,11 @@ class FSWatcher extends EventEmitter {
if (file.isFile()) {
this.#watchFile(f);
} else if (file.isDirectory() && !file.isSymbolicLink()) {
await this.#watchFolder(f);
Copy link
Member

Choose a reason for hiding this comment

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

Moving to Promise.all, this code will not probably throw EMFILE, too many open files?

@BridgeAR BridgeAR added the needs-benchmark-ci PR that need a benchmark CI run. label Jan 3, 2024
@BridgeAR
Copy link
Member

BridgeAR commented Jan 3, 2024

Do we have a good benchmark for this? I am surprised that this should improve the performance, I would actually expect it to be identical. In deno, there were a few changes that made it run in parallel but this should already execute things in parallel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants