Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: make fs.read params optional #31402

Closed

Conversation

lholmquist
Copy link
Contributor

@lholmquist lholmquist commented Jan 17, 2020

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

This makes all the parameters of the fs.read function, except
for fd and the callback(when not using as a promise) optional.

fixes #31237

I still need to add a few tests(i've done some manual testing), but wanted to get this out to start the discussion.

There is one part, which i've commented in the code, where i did something to satisfy the linting, but i wasn't thrilled with it. Maybe having a lint exception here might be better? Looking for input on that

I'm not a typescript user, but i wonder if changing the function signature will somehow have a negative effect

This makes all the parameters of the `fs.read` function, except
for `fd` and the callback(when not using as a promise) optional.

They will default to sensible defaults

fixes nodejs#31237
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jan 17, 2020
@mscdex
Copy link
Contributor

mscdex commented Jan 17, 2020

I don't think it's that easy, especially since there are 3 consecutive integer parameters and trying to determine which was specified if there were less than 3 integers passed is impossible. I think we'd only be able to (reliably) support the following cases:

fs.read(fd, buffer, offset, length, position, callback);
fs.read(fd, buffer, callback);
fs.read(fd, callback);

For the last signature, the documentation must be updated to clearly indicate the default buffer size.

Additionally, I think the language features used here may cause performance issues so it'd be better to just stick to the traditional type checking.

@lholmquist
Copy link
Contributor Author

I don't think it's that easy, especially since there are 3 consecutive integer parameters and trying to determine which was specified if there were less than 3 integers passed is impossible.

I see what you are saying, but i think the assumption is(and maybe it is just my assumption), that when you want to specify, for example length, that you would also need to specify any params before it. So we should know what param is being passed in since they are positional

So in that case the signatures would look like this:

fs.read(fd, buffer, offset, length, position, callback);
fs.read(fd, buffer, offset, length, callback);
fs.read(fd, buffer, offset, callback);
fs.read(fd, buffer, callback);
fs.read(fd, callback);

at least that is how i read the usage from the fs.write method that i thought was doing something similar:
https://github.com/nodejs/node/pull/31402/files#diff-9a205ef7ee967ee32efee02e58b3482dR547

If that is not the case, i'm happy to amend this to just do the signatures from the above post

Additionally, I think the language features used here may cause performance issues so it'd be better to just stick to the traditional type checking.

Which features? spread? performance isn't really in my wheel house, so i don't mind updating if that is the best way to go

@mscdex
Copy link
Contributor

mscdex commented Jan 18, 2020

Which features? spread? performance isn't really in my wheel house, so i don't mind updating if that is the best way to go

Spread in the parameter list may be ok, but last I checked destructuring and such caused performance regressions.

@ronag
Copy link
Member

ronag commented Jan 18, 2020

Spread in the parameter list may be ok, but last I checked destructuring and such caused performance regressions.

Might have been fixed in V8 7.8 (#30109).

@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Jan 20, 2020
lib/fs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
@lholmquist
Copy link
Contributor Author

I'm sort of drawing a blank on what other test should be added here. The only one I can think of, that wouldn't be duplicating something from an already existing test, is testing that the default buffer that is created is the correct size. I've added that one in the latest commit

@ronag ronag removed the wip Issues and PRs that are still a work in progress. label Jan 27, 2020
@ronag
Copy link
Member

ronag commented Jan 27, 2020

doc/api/fs.md Outdated
@@ -2716,10 +2716,14 @@ Returns an integer representing the file descriptor.
For detailed information, see the documentation of the asynchronous version of
this API: [`fs.open()`][].

## `fs.read(fd, buffer, offset, length, position, callback)`
## `fs.read(fd, [buffer[, [offset[, length[, position]]]], callback)`
Copy link
Member

Choose a reason for hiding this comment

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

As much as I dislike polymorphic signatures, I'd much prefer the approach of moving to an options object as an alternative here. That is:

fs.read(fd, { offset: n, position: n }, callback)

It accomplishes the same goal with a much cleaner API and implementation, without the ambiguity of which argument was meant to be passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell I think i see what you are saying. Just for my own clarification, we keep the current signature of fs.read(fd, buffer, offset, lenght, position, callback), but then add another signature of fs.read(fd, options, callback) where the options is the buffer, offset, length, position params

So then if a user only wanted to specify some of the params, they would have to use the "options" object signature.

Does that sound correct?

One thing that jumps out at me here, is when checking to see if that second parameter is the options object instead of the buffer object, i don't think we can use typeof since both would return object. Or am i overthinking this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell Something else that i thought of was that the fs.write method is similar, in that it allows for optional parameters to be passed without the use of an options object.

Perhaps we don't go the options object route? Or if we do, maybe that function and others(not sure if there are)should be updated also?

Copy link
Member

Choose a reason for hiding this comment

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

I think moving that direction is ideal but doesn't have to be done all at once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell @ronag does what i said here, #31402 (comment) makes sense?

@jasnell
Copy link
Member

jasnell commented Jan 27, 2020

For API symmetry, this should be done also on FileHandle.prototype.read() in the fs.promises API (https://nodejs.org/dist/latest-v13.x/docs/api/fs.html#fs_filehandle_read_buffer_offset_length_position)

@lholmquist
Copy link
Contributor Author

lholmquist commented Jan 28, 2020

For API symmetry, this should be done also on FileHandle.prototype.read() in the fs.promises API

Yea, definitely. I'll probably send this in a second PR just to keep things clean

@lholmquist
Copy link
Contributor Author

lholmquist commented Jan 28, 2020

For API symmetry, this should be done also on FileHandle.prototype.read() in the fs.promises API

Yea, definitely. I'll probably send this in a second PR just to keep things clean

I just realized that these are just doc updates, so nvm, i can add to this pr 🤦‍♂️

@ronag
Copy link
Member

ronag commented Jan 28, 2020

Benchmark results:

                                                                                 confidence improvement accuracy (*)    (**)   (***)
 fs/readfile.js concurrent=10 len=1024 dur=5                                                     -0.35 %       ±5.12%  ±6.81%  ±8.87%
 fs/readfile.js concurrent=10 len=16777216 dur=5                                                 -0.51 %       ±3.46%  ±4.60%  ±5.99%
 fs/readfile.js concurrent=1 len=1024 dur=5                                                      -2.40 %       ±3.09%  ±4.12%  ±5.36%
 fs/readfile.js concurrent=1 len=16777216 dur=5                                                  -9.42 %      ±16.17% ±21.51% ±28.00%
 fs/readfile-partitioned.js concurrent=10 len=1024 dur=5                                         -1.92 %       ±5.05%  ±6.73%  ±8.79%
 fs/readfile-partitioned.js concurrent=10 len=16777216 dur=5                                     -2.00 %       ±4.07%  ±5.41%  ±7.04%
 fs/readfile-partitioned.js concurrent=1 len=1024 dur=5                                           2.70 %       ±6.40%  ±8.51% ±11.08%
 fs/readfile-partitioned.js concurrent=1 len=16777216 dur=5                                       0.15 %       ±3.23%  ±4.30%  ±5.61%
 fs/readFileSync.js n=600000                                                                     -0.63 %       ±8.40% ±11.20% ±14.63%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='asc'                    2.04 %       ±4.35%  ±5.79%  ±7.54%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='buf'             *      7.80 %       ±6.13%  ±8.17% ±10.65%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='utf'                    0.34 %       ±3.51%  ±4.67%  ±6.07%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='asc'         **      8.79 %       ±6.30%  ±8.47% ±11.20%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='buf'                -4.63 %      ±10.22% ±13.62% ±17.77%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='utf'                 6.04 %      ±19.15% ±25.48% ±33.17%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='asc'                    1.61 %       ±5.53%  ±7.35%  ±9.57%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='buf'                    1.86 %       ±4.46%  ±5.93%  ±7.72%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='utf'             *     -6.08 %       ±4.60%  ±6.14%  ±8.02%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='asc'                  -3.43 %      ±10.17% ±13.53% ±17.62%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='buf'                   3.28 %       ±9.10% ±12.11% ±15.76%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='utf'                  -6.38 %       ±7.26%  ±9.69% ±12.66%

@lholmquist
Copy link
Contributor Author

lholmquist commented Feb 4, 2020

@jasnell @ronag just a friendly ping, i think my last comment may have gotten lost in the shuffle but does what i said here, #31402 (comment) make sense?

@jasnell
Copy link
Member

jasnell commented Feb 4, 2020

Hey sorry, I missed the earlier notification. I'd really prefer if the changes to fs.promise were in a separate commit in the same PR. Those apis should remain consistent and having it in a separate pr could make things more difficult

@lholmquist
Copy link
Contributor Author

I'd really prefer if the changes to fs.promise were in a separate commit in the same PR. Those apis should remain consistent and having it in a separate pr could make things more difficult

Yea, i realized after i wrote that, it was just doc updates.

I guess my main question was about my thought process here:

I think i see what you are saying. Just for my own clarification, we keep the current signature of fs.read(fd, buffer, offset, lenght, position, callback), but then add another signature of fs.read(fd, options, callback) where the options is the buffer, offset, length, position params

So then if a user only wanted to specify some of the params, they would have to use the "options" object signature.

Does that sound correct?

@jasnell
Copy link
Member

jasnell commented Feb 4, 2020

but then add another signature of fs.read(fd, options, callback)

Ah, right... yes, that's exactly what I'm suggesting. It gives significantly more flexibility in how those various other properties are handled.

now using the options object instead of making all things optional
@lholmquist
Copy link
Contributor Author

I've added a new commit that adds the signature fs.read(fs, options, callback) . just wanted to get some feedback first. I'm thinking the options object can also be optional, but i haven't coded that part yet

doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
lholmquist and others added 3 commits February 12, 2020 08:04
Co-Authored-By: Richard Lau <riclau@uk.ibm.com>
Co-Authored-By: Richard Lau <riclau@uk.ibm.com>
@lholmquist
Copy link
Contributor Author

I added the optional options um...... option, so fs.read(fd, callback) will now work,

doc/api/fs.md Show resolved Hide resolved
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I think

test-fs-read-optional-options-params.js
test/parallel/test-fs-read-optional-params.js

can be just one file

@lholmquist
Copy link
Contributor Author

I think
test-fs-read-optional-options-params.js
test/parallel/test-fs-read-optional-params.js
can be just one file

Yea, probably. when i was writing those, i had them in the same file, but i was having an issue because i was re-using the same file. i'll try to rework it to make it work

@lholmquist
Copy link
Contributor Author

@ronag I added another test and combined the 2 tests files into 1

@lholmquist lholmquist requested a review from ronag March 9, 2020 14:32
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

LGTM

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 10, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 11, 2020
addaleax pushed a commit that referenced this pull request Mar 11, 2020
This makes all the parameters of the `fs.read` function, except
for `fd` and the callback(when not using as a promise) optional.

They will default to sensible defaults.

Fixes: #31237

PR-URL: #31402
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@addaleax
Copy link
Member

Landed in b6da55f

@addaleax addaleax closed this Mar 11, 2020
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
This makes all the parameters of the `fs.read` function, except
for `fd` and the callback(when not using as a promise) optional.

They will default to sensible defaults.

Fixes: #31237

PR-URL: #31402
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@MylesBorins MylesBorins mentioned this pull request Mar 12, 2020
* `buffer` {Buffer|Uint8Array} **Default:** `Buffer.alloc(16384)`
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.length`
* `position` {integer} **Default:** `null`
Copy link
Member

Choose a reason for hiding this comment

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

This is not the same as the actual. options.position is the undefined when default

* `buffer` {Buffer|TypedArray|DataView} **Default:** `Buffer.alloc(16384)`
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.length`
* `position` {integer} **Default:** `null`
Copy link
Member

Choose a reason for hiding this comment

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

same here

@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
This makes all the parameters of the `fs.read` function, except
for `fd` and the callback(when not using as a promise) optional.

They will default to sensible defaults.

Fixes: nodejs#31237

PR-URL: nodejs#31402
Reviewed-By: Robert Nagy <ronagy@icloud.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
This makes all the parameters of the `fs.read` function, except
for `fd` and the callback(when not using as a promise) optional.

They will default to sensible defaults.

Fixes: #31237

PR-URL: #31402
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

fs: make read parameters optional
10 participants