Skip to content
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

fs: use streaming directory processing in cp() #41351

Merged
merged 2 commits into from
Dec 31, 2021

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 29, 2021

The readdir() functions do not scale well, which is why opendir(), etc. were introduced. This is exacerbated in the current cp() implementation, which calls readdir() recursively.

This commit updates cp() to use the opendir() style iteration.

The readdir() functions do not scale well, which is why
opendir(), etc. were introduced. This is exacerbated in the
current cp() implementation, which calls readdir() recursively.

This commit updates cp() to use the opendir() style iteration.
@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 29, 2021
@bnb
Copy link
Contributor

bnb commented Dec 29, 2021

Would love to see perf metrics on this if you've got any handy 👀

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

lib/internal/fs/cp/cp.js Outdated Show resolved Hide resolved
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 29, 2021

Would love to see perf metrics on this if you've got any handy

I don't have any. The readdir() based implementation should actually be faster.

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 29, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 29, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@Mesteery Mesteery added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 29, 2021
@cjihrig cjihrig changed the title fs: use async directory processing in cp() fs: use streaming directory processing in cp() Dec 29, 2021
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

Awesome!

@Mesteery Mesteery added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 30, 2021
@cjihrig cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 31, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 31, 2021
@nodejs-github-bot nodejs-github-bot merged commit d0c1176 into nodejs:master Dec 31, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in d0c1176

targos pushed a commit that referenced this pull request Jan 14, 2022
The readdir() functions do not scale well, which is why
opendir(), etc. were introduced. This is exacerbated in the
current cp() implementation, which calls readdir() recursively.

This commit updates cp() to use the opendir() style iteration.

PR-URL: #41351
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
The readdir() functions do not scale well, which is why
opendir(), etc. were introduced. This is exacerbated in the
current cp() implementation, which calls readdir() recursively.

This commit updates cp() to use the opendir() style iteration.

PR-URL: #41351
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
The readdir() functions do not scale well, which is why
opendir(), etc. were introduced. This is exacerbated in the
current cp() implementation, which calls readdir() recursively.

This commit updates cp() to use the opendir() style iteration.

PR-URL: nodejs#41351
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
The readdir() functions do not scale well, which is why
opendir(), etc. were introduced. This is exacerbated in the
current cp() implementation, which calls readdir() recursively.

This commit updates cp() to use the opendir() style iteration.

PR-URL: #41351
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
SukkaW added a commit to SukkaW/node-fs-extra that referenced this pull request Nov 28, 2023
SukkaW added a commit to SukkaW/node-fs-extra that referenced this pull request Nov 28, 2023
RyanZim pushed a commit to jprichardson/node-fs-extra that referenced this pull request Feb 10, 2024
* refactor(copy): backport nodejs/node#41351

* perf(copy): parallel copy

* perf(copy): run filter in parallel as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants