FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: add `bufferSize` option to `fs.opendir()` by addaleax · Pull Request #30114 · nodejs/node · GitHub

/ node Public

fs: add bufferSize option to fs.opendir() - #30114

Closed
addaleax wants to merge 7 commits into
nodejs:masterfrom
addaleax:fs-opendir-buffersize
ConversationCommits (7)ChecksFiles changed
Closed

fs: add bufferSize option to fs.opendir()#30114
addaleax wants to merge 7 commits into
nodejs:masterfrom
addaleax:fs-opendir-buffersize

Conversation

addaleax commented Oct 25, 2019

Copy link
Copy Markdown
Member

Add an option that controls the size of the internal
buffer.

Fixes: #29941

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

fs: add bufferSize option to fs.opendir()
ac9b36b 
Add an option that controls the size of the internal
buffer.

Fixes: nodejs#29941
nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Oct 25, 2019

duffleit commented Oct 25, 2019

Copy link
Copy Markdown

What a mob programming session 🙌🏻😀

jdjuan commented Oct 25, 2019

Copy link
Copy Markdown

300 people watched the process of this PR during @scriptconf Great job!!!!!

richardlau reviewed Oct 25, 2019
View reviewed changes
Comment thread doc/api/fs.md Outdated
Comment thread doc/api/fs.md Outdated
Comment thread doc/api/fs.md Outdated
Comment thread doc/api/fs.md Outdated
Comment thread doc/api/fs.md Outdated
Comment thread doc/api/fs.md Outdated

drdreo commented Oct 25, 2019

Copy link
Copy Markdown

Can approve that all tests were done

nodejs-github-bot commented Oct 25, 2019

Copy link
Copy Markdown
Collaborator

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

Trott commented Oct 25, 2019

Copy link
Copy Markdown
Member

bufferSize will need to be added to test/benchmark/test-benchmark-fs.js

Speaking of which: Needs tests. (Just forgot to git add them maybe? You checked the box for tests and I know it's not like you to not have tests. :-D )

cjihrig reviewed Oct 25, 2019
View reviewed changes
Comment thread doc/api/fs.md Outdated

addaleax commented Oct 25, 2019

Copy link
Copy Markdown
Member Author

bufferSize will need to be added to test/benchmark/test-benchmark-fs.js

Done, thanks!

Speaking of which: Needs tests. (Just forgot to git add them maybe? You checked the box for tests and I know it's not like you to not have tests. :-D )

I checked the box because I added benchmarks that measure the performance impact – I don’t think this is something that we could test for well without digging into internals.

addaleax requested a review from Fishrock123 October 25, 2019 20:39
addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 25, 2019

richardlau commented Oct 25, 2019

Copy link
Copy Markdown
Member

bufferSize will need to be added to test/benchmark/test-benchmark-fs.js

Done, thanks!

Speaking of which: Needs tests. (Just forgot to git add them maybe? You checked the box for tests and I know it's not like you to not have tests. :-D )

I checked the box because I added benchmarks that measure the performance impact – I don’t think this is something that we could test for well without digging into internals.

Test passing non-numbers/negative numbers/zero/non-integers as bufferSize?

Trott commented Oct 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Test passing non-numbers/negative numbers/zero/non-integers as bufferSize?

And maybe one test for 32 and one other reasonable positive integer to confirm that it doesn't throw in those cases?

addaleax commented Oct 25, 2019

Copy link
Copy Markdown
Member Author

@Trott @richardlau I guess, yes – added tests for the error condition/that it works when using a positive integer value.

richardlau approved these changes Oct 25, 2019
View reviewed changes
Comment thread test/parallel/test-fs-opendir.js Outdated
fixup! fs: add bufferSize option to fs.opendir()
2119b4c 
Co-Authored-By: Richard Lau <riclau@uk.ibm.com>
addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 26, 2019

nodejs-github-bot commented Oct 26, 2019

Copy link
Copy Markdown
Collaborator

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

cjihrig approved these changes Oct 26, 2019
View reviewed changes
Comment thread lib/internal/fs/dir.js Outdated

nodejs-github-bot commented Oct 26, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Oct 28, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Oct 28, 2019

Copy link
Copy Markdown
Collaborator

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

addaleax commented Oct 28, 2019

Copy link
Copy Markdown
Member Author

Landed in b35181f 🎉

addaleax closed this Oct 28, 2019
addaleax added a commit that referenced this pull request Oct 28, 2019
fs: add bufferSize option to fs.opendir()
b35181f 
Add an option that controls the size of the internal
buffer.

Fixes: #29941

PR-URL: #30114
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax deleted the fs-opendir-buffersize branch October 28, 2019 13:35
This was referenced Oct 31, 2019
Closed
Closed
targos pushed a commit that referenced this pull request Nov 5, 2019
fs: add bufferSize option to fs.opendir()
07b5584 
Add an option that controls the size of the internal
buffer.

Fixes: #29941

PR-URL: #30114
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos added a commit that referenced this pull request Nov 5, 2019
2019-11-05, Version 13.1.0 (Current)
64eacd8 
Notable changes:

* cli:
  * Added a new flag (`--trace-uncaught`) that makes Node.js print the
    stack trace at the time of throwing uncaught exceptions, rather than
    at the creation of the `Error` object, if there is any. This is
    disabled by default because it affects GC behavior.
    #30025
* crypto
  * Added `Hash.prototype.copy()` method. It returns a new `Hash` object
    with its internal state cloned from the original one.
    #29910
* dgram
  * Added source-specific multicast support. This adds methods to
    Datagram sockets to support RFC 4607
    (https://tools.ietf.org/html/rfc4607) for IPv4 and IPv6.
    #15735
* fs
  * Added a `bufferSize` option to `fs.opendir()`. It allows to control
    the number of entries that are buffered internally when reading from
    the directory. #30114
* meta
  * Added Chengzhong Wu (https://github.com/legendecas) to
    collaborators. #30115

PR-URL: #30262
targos mentioned this pull request Nov 5, 2019
Merged
targos added a commit that referenced this pull request Nov 5, 2019
2019-11-05, Version 13.1.0 (Current)
309e1ea 
Notable changes:

* cli:
  * Added a new flag (`--trace-uncaught`) that makes Node.js print the
    stack trace at the time of throwing uncaught exceptions, rather than
    at the creation of the `Error` object, if there is any. This is
    disabled by default because it affects GC behavior.
    #30025
* crypto
  * Added `Hash.prototype.copy()` method. It returns a new `Hash` object
    with its internal state cloned from the original one.
    #29910
* dgram
  * Added source-specific multicast support. This adds methods to
    Datagram sockets to support RFC 4607
    (https://tools.ietf.org/html/rfc4607) for IPv4 and IPv6.
    #15735
* fs
  * Added a `bufferSize` option to `fs.opendir()`. It allows to control
    the number of entries that are buffered internally when reading from
    the directory. #30114
* meta
  * Added Chengzhong Wu (https://github.com/legendecas) to
    collaborators. #30115

PR-URL: #30262
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
fs: add bufferSize option to fs.opendir()
ba5683c 
Add an option that controls the size of the internal
buffer.

Fixes: #29941

PR-URL: #30114
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
fs: add bufferSize option to fs.opendir()
785aa86 
Add an option that controls the size of the internal
buffer.

Fixes: #29941

PR-URL: #30114
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Feb 8, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Trott Trott approved these changes
cjihrig cjihrig approved these changes
richardlau richardlau approved these changes
Fishrock123 Awaiting requested review from Fishrock123

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fs.Dir.read() is very slow

10 participants

Back | FazBrowse Home | New Git URL