GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: use streaming directory processing in cp() by cjihrig · Pull Request #41351 · nodejs/node

Conversation

@cjihrig

@cjihrig cjihrig commented Dec 29, 2021

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss, styfle, and gioragutt reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@cjihrig
fs: use async directory processing in cp()
b5aa0b3 
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 file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Dec 29, 2021
@bnb

bnb commented Dec 29, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Dec 29, 2021
View reviewed changes
bnb
bnb approved these changes Dec 29, 2021
View reviewed changes
mcollina
mcollina approved these changes Dec 29, 2021
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lpinca
lpinca reviewed Dec 29, 2021
View reviewed changes
Comment thread lib/internal/fs/cp/cp.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Dec 29, 2021
View reviewed changes
@cjihrig

cjihrig commented Dec 29, 2021

Copy link
Copy Markdown
Contributor Author

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.
bnb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig @lpinca
Update lib/internal/fs/cp/cp.js
625c12f 
Co-authored-by: Luigi Pinca <[email protected]>
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 29, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 29, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Dec 29, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41681/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Mesteery Mesteery added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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
juanarbol
juanarbol approved these changes Dec 29, 2021
View reviewed changes

@juanarbol juanarbol left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Awesome!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@github-actions github-actions Bot mentioned this pull request Dec 30, 2021
CI Reliability 2021-12-30 nodejs/reliability#160
Open
27 tasks
@Mesteery Mesteery added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 30, 2021
VoltrexKeyva
VoltrexKeyva approved these changes Dec 30, 2021
View reviewed changes
@github-actions github-actions Bot mentioned this pull request Dec 31, 2021
CI Reliability 2021-12-31 nodejs/reliability#161
Open
26 tasks
@cjihrig cjihrig added the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 31, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 31, 2021
@nodejs-github-bot
nodejs-github-bot merged commit d0c1176 into nodejs:master Dec 31, 2021
@nodejs-github-bot

nodejs-github-bot commented Dec 31, 2021

Copy link
Copy Markdown
Collaborator

Landed in d0c1176
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig
cjihrig deleted the cp branch December 31, 2021 20:47
This was referenced Jan 1, 2022
CI Reliability 2022-01-01 nodejs/reliability#162
Open
CI Reliability 2022-01-02 nodejs/reliability#163
Open
CI Reliability 2022-01-03 nodejs/reliability#164
Open
This was referenced Jan 4, 2022
CI Reliability 2022-01-04 nodejs/reliability#165
Open
CI Reliability 2022-01-05 nodejs/reliability#166
Open
CI Reliability 2022-01-06 nodejs/reliability#167
Open
CI Reliability 2022-01-07 nodejs/reliability#168
Open
targos pushed a commit that referenced this pull request Jan 14, 2022
@cjihrig @targos
fs: use async directory processing in cp()
d4a6f2c 
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 <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
@targos targos mentioned this pull request Jan 16, 2022
v17.4.0 release proposal #41557
Merged
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@cjihrig @danielleadams
fs: use async directory processing in cp()
285d3a6 
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 <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@cjihrig @danielleadams
fs: use async directory processing in cp()
66556b3 
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 <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
v16.14.0 proposal #41804
Merged
@RyanZim RyanZim mentioned this pull request Oct 21, 2022
Use fs.opendir() in copy*() jprichardson/node-fs-extra#972
Closed
SukkaW added a commit to SukkaW/node-fs-extra that referenced this pull request Nov 28, 2023
@SukkaW
refactor(copy): backport nodejs/node#41351
ac14e53
SukkaW added a commit to SukkaW/node-fs-extra that referenced this pull request Nov 28, 2023
@SukkaW
refactor(copy): backport nodejs/node#41351
1a80ec5
RyanZim pushed a commit to jprichardson/node-fs-extra that referenced this pull request Feb 10, 2024
@SukkaW
Refactor copy to use opendir (#1028)
1d931c8 
* refactor(copy): backport nodejs/node#41351

* perf(copy): parallel copy

* perf(copy): run filter in parallel as well
neysidevhcr1j added a commit to neysidevhcr1j/neysidevhcr1j that referenced this pull request May 20, 2025
@neysidevhcr1j
Refactor copy to use opendir (#1028)
b7c5b9c 
* refactor(copy): backport nodejs/node#41351

* perf(copy): parallel copy

* perf(copy): run filter in parallel as well
criticalPathway pushed a commit to criticalPathway/node-fs-extra that referenced this pull request Nov 17, 2025
@SukkaW
Refactor copy to use opendir (#1028)
df48bad 
* 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

Reviewers

@lpinca lpinca lpinca approved these changes
@mcollina mcollina mcollina approved these changes
@bnb bnb bnb approved these changes
@addaleax addaleax addaleax approved these changes
@juanarbol juanarbol juanarbol approved these changes
+1 more reviewer
@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@cjihrig @bnb @nodejs-github-bot @mcollina @addaleax @lpinca @juanarbol @VoltrexKeyva @Mesteery
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.