Skip to content

Conversation

@andersk
Copy link
Contributor

@andersk andersk commented Nov 22, 2019

This makes walking large directory trees much more efficient on Node 10.10 or later.

See:
https://lwn.net/Articles/606995/
https://www.python.org/dev/peps/pep-0471/
nodejs/node#22020
https://nodejs.org/en/blog/release/v10.10.0/

@msftclas
Copy link

msftclas commented Nov 22, 2019

CLA assistant check
All CLA requirements met.

This makes walking large directory trees much more efficient on Node
10.10 or later.

See:
https://lwn.net/Articles/606995/
https://www.python.org/dev/peps/pep-0471/
nodejs/node#22020
https://nodejs.org/en/blog/release/v10.10.0/

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Love it. Only suggestion is that we might want to do a single check up front, rather than looking for strings in every result. See #36139.

function getDirectories(path: string): string[] {
perfLogger.logEvent("ReadDir: " + path);
return filter<string>(_fs.readdirSync(path), dir => fileSystemEntryExists(combinePaths(path, dir), FileSystemEntryKind.Directory));
return getAccessibleFileSystemEntries(path).directories.slice();
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to slice since getAccessibleFileSystemEntries returns new array for directories..

Copy link
Contributor Author

@andersk andersk Jan 13, 2020

Choose a reason for hiding this comment

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

Not always: return emptyFileSystemEntries does not. Of course, that’s easy to change. I’ve added a commit doing so.

Edit: I’ve now removed that commit again because we didn’t want to change the type of getAccessibleFileSystemEntries (at least not in this PR); see this discussion.

@andersk andersk force-pushed the readdir-withFileTypes branch 2 times, most recently from 2b983fa to 01aec72 Compare January 13, 2020 20:31
@andersk
Copy link
Contributor Author

andersk commented Jan 13, 2020

Only suggestion is that we might want to do a single check up front, rather than looking for strings in every result.

My reasoning was that the typeof checks are extremely cheap compared to the IO we’re doing (even now), and doing the check up front requires duplicating code. Let me know if you’d like me to do this anyway.

@amcasey
Copy link
Member

amcasey commented Jan 13, 2020

@andersk I don't feel strongly either way. I agree that it's a small overhead, I just thought it seemed like a shame to pay it for all future versions (whereas an explicit version check is more likely to be cleaned up if we decide to deprecate older nodes). Either way, this is a huge improvement over the current implementation. Thanks again!

@amcasey
Copy link
Member

amcasey commented Jan 13, 2020

The CI failure is unrelated. We're working on it.

@amcasey
Copy link
Member

amcasey commented Jan 14, 2020

Closing and re-opening to re-trigger CI.

@amcasey amcasey closed this Jan 14, 2020
@amcasey amcasey reopened this Jan 14, 2020
@andersk andersk force-pushed the readdir-withFileTypes branch from 01aec72 to bee7ee0 Compare January 15, 2020 03:54
@amcasey amcasey merged commit 64704a1 into microsoft:master Jan 15, 2020
@amcasey
Copy link
Member

amcasey commented Jan 15, 2020

Thanks, @andersk! My local benchmarking suggests this will make a big difference. And sorry about the delay in getting eyes on your PR.

Kingwl pushed a commit to Kingwl/TypeScript that referenced this pull request Mar 4, 2020
@andersk andersk deleted the readdir-withFileTypes branch October 15, 2022 22:22
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants