Skip to content

fs: make recursive readdir algorithms iterative #47650

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 6 commits into from
May 10, 2023
Merged

fs: make recursive readdir algorithms iterative #47650

merged 6 commits into from
May 10, 2023

Conversation

Ethan-Arrowood
Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood commented Apr 20, 2023 •
edited
Loading

This PR updates the new readdir recursive algorithm to be iterative. This helps avoid potential callstack issues for very large directories.

@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 Apr 20, 2023
@Ethan-Arrowood Ethan-Arrowood force-pushed the improve-recursive-algs branch from 7de1599 to 28fc566 Compare April 20, 2023 19:46
@Ethan-Arrowood Ethan-Arrowood changed the title fs: make recursive readdir and opendir algorithms iterative fs: make recursive readdir algorithms iterative Apr 20, 2023
@Ethan-Arrowood Ethan-Arrowood marked this pull request as ready for review April 20, 2023 19:48
anonrig
anonrig reviewed Apr 20, 2023
View reviewed changes
@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 20, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51388/

@anonrig anonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 20, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51390/

mscdex
mscdex reviewed Apr 21, 2023
View reviewed changes
@Ethan-Arrowood @mscdex
Update lib/fs.js
57f16be 
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
aduh95
aduh95 reviewed Apr 22, 2023
View reviewed changes
@github-actions github-actions bot mentioned this pull request Apr 24, 2023
Open
39 tasks
@Ethan-Arrowood @aduh95
Update lib/fs.js
36bad0f 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95
aduh95 reviewed Apr 24, 2023
View reviewed changes
mscdex
mscdex reviewed Apr 24, 2023
View reviewed changes
mscdex
mscdex reviewed Apr 24, 2023
View reviewed changes
@github-actions github-actions bot mentioned this pull request Apr 25, 2023
Open
38 tasks
Ethan-Arrowood and others added 2 commits April 25, 2023 10:47
@Ethan-Arrowood @aduh95
Update lib/fs.js
8803165 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
make it even faster
679d8cd
@Ethan-Arrowood
Copy link
Contributor Author

Thank you for the great recommendations @mscdex

I have made all the changes and thought I haven't timed it, the tests are running noticeably faster on my local machine 😄

@github-actions github-actions bot mentioned this pull request Apr 26, 2023
Open
36 tasks
@Ethan-Arrowood
Copy link
Contributor Author

Is there anything blocking this from merging?

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51720/

@anonrig
Copy link
Member

anonrig commented May 9, 2023

Since 7 days have passed since you opened this PR, we can merge this with only 1 review, but I prefer to wait a couple of days to receive a review from @nodejs/fs team.

@MoLow
Copy link
Member

MoLow commented May 9, 2023

Since 7 days have passed since you opened this PR, we can merge this with only 1 review, but I prefer to wait a couple of days to receive a review from @nodejs/fs team.

I will take a look later

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51729/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51734/

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label May 10, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 10, 2023
@nodejs-github-bot nodejs-github-bot merged commit 12a93ce into nodejs:main May 10, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 12a93ce

This was referenced May 11, 2023
Open
Open
targos pushed a commit that referenced this pull request May 12, 2023
@targos
fs: make readdir recursive algorithm iterative
106dc61 
PR-URL: #47650
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
This was referenced May 13, 2023
Open
Open
Open
@targos targos mentioned this pull request May 15, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@danielleadams
fs: make readdir recursive algorithm iterative
8e9a78b 
PR-URL: #47650
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@MoLow
fs: make readdir recursive algorithm iterative
9b44c56 
PR-URL: nodejs#47650
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
@juanarbol juanarbol mentioned this pull request Nov 4, 2024
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@mscdex mscdex mscdex left review comments

@aduh95 aduh95 aduh95 left review comments

@jasnell jasnell jasnell approved these changes

@anonrig anonrig anonrig approved these changes

@MoLow MoLow MoLow approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Ethan-Arrowood @nodejs-github-bot @anonrig @MoLow @mscdex @jasnell @aduh95