Uh oh!
There was an error while loading. Please reload this page.
sys: Use readdir withFileTypes option to skip lots of stat syscalls - #35286
Conversation
|
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#22020https://nodejs.org/en/blog/release/v10.10.0/ Signed-off-by: Anders Kaseorg <andersk@mit.edu>
96a40c3 to
bee7ee0Compare
Andrew Casey (amcasey)
left a comment
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
You don't need to slice since getAccessibleFileSystemEntries returns new array for directories..
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
2b983fa to
01aec72CompareAnders Kaseorg (andersk)
commented
Jan 13, 2020
My reasoning was that the |
Andrew Casey (amcasey)
commented
Jan 13, 2020
Anders Kaseorg (@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! |
Andrew Casey (amcasey)
commented
Jan 13, 2020
The CI failure is unrelated. We're working on it. |
Uh oh!
There was an error while loading. Please reload this page.
Andrew Casey (amcasey)
commented
Jan 14, 2020
Closing and re-opening to re-trigger CI. |
01aec72 to
bee7ee0CompareAndrew Casey (amcasey)
commented
Jan 15, 2020
Thanks, Anders Kaseorg (@andersk)! My local benchmarking suggests this will make a big difference. And sorry about the delay in getting eyes on your PR. |
…icrosoft#35286) 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#22020https://nodejs.org/en/blog/release/v10.10.0/ Signed-off-by: Anders Kaseorg <andersk@mit.edu>
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/