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.

Sign up for GitHub

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

Jump to bottom

fs: added fs.writev() which exposes syscalls writev() #25925

Closed
wants to merge 3 commits into from
Closed

fs: added fs.writev() which exposes syscalls writev() #25925

wants to merge 3 commits into from

Conversation

AnasAboreeda
Copy link
Contributor

fs with writev allow many buffers to be pushed to underlying OS
APIs in one batch, so this should improve write speed to files.

I have tried to follow the old fs.write() function signature
and make all the features of it available when users use fs.writev()

Fixes: #2298

: expose-syscall-writev-to-fs

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

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. tools Issues and PRs related to the tools directory. labels Feb 4, 2019
mscdex
mscdex reviewed Feb 4, 2019
View reviewed changes
lib/fs.js Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Feb 4, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Feb 4, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented Feb 4, 2019

I don't understand why if a single buffer object is passed, it would be split up in chunks just to use binding.writeBuffers()? That seems inefficient. I would expect the function to throw an error on a non-array or an array containing a non-Buffer-like object.

@AnasAboreeda
Copy link
Contributor Author

@mscdex I am trying to support all types that are supported by fs.write(). Do you think it's a good idea to just support buffer[] as an input?

@mscdex
Copy link
Contributor

mscdex commented Feb 4, 2019

Do you think it's a good idea to just support buffer[] as an input?

I don't see a reason to restrict it like that. Any data type supported by the language or node.js that stores binary data should be allowed IMO.

On another note, I think if we are going to support strings, I think it might be better to support them on the C++ side like we do for socket writev to avoid having to possibly cross the JS/C++ boundary for each string. One thing that you might want to benchmark though with that C++ implementation is whether it'd be faster to do the allBuffers check in JS first or just always have the C++ check the types itself (throwing an error when a non-string and non-buffer is found).

@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. and removed tools Issues and PRs related to the tools directory. labels Feb 4, 2019
@addaleax
Copy link
Member

addaleax commented Feb 4, 2019

I would suggest supporting ArrayBufferView[] for now; it doesn’t have to be just Buffers, but supporting a single Buffer or strings is probably not the best idea for now, especially if we’d convert them anyway.

bnoordhuis
bnoordhuis reviewed Feb 5, 2019
View reviewed changes
lib/fs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
test/parallel/test-fs-writev.js Outdated Show resolved Hide resolved
@AnasAboreeda
Copy link
Contributor Author

@addaleax & @mscdex Now fs.writev() only accepts an array of chunks as an input. Can you please check again?

addaleax
addaleax approved these changes Feb 6, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Nice work!

lib/fs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
test/parallel/test-fs-writev.js Outdated Show resolved Hide resolved
tools/doc/type-parser.js Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Feb 6, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Feb 6, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Feb 6, 2019
View reviewed changes
lib/fs.js Outdated
@@ -591,6 +604,36 @@ function writeSync(fd, buffer, offset, length, position) {
return result;
}

// usage:
// fs.writev(fd, buffers, [position], callback);
function writev(fd, buffer, 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.

A Promisified version of this should be added to fs.promises also

Copy link
Contributor

Choose a reason for hiding this comment

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

The parameter name should be buffers to match the comment and the intention.

mscdex
mscdex reviewed Feb 7, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Feb 7, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Feb 7, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented Feb 7, 2019 •
edited
Loading

I think we should probably also add fs.writevSync() for completeness.

targos pushed a commit that referenced this pull request Aug 19, 2019
@AnasAboreeda @targos
fs: add fs.writev() which exposes syscall writev()
bb19d82 
fs with writev allow many buffers to be pushed to underlying OS
APIs in one batch, so this should improve write speed to files.

Refs: #2298

PR-URL: #25925
Fixes: #2298
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Aug 19, 2019
Merged
targos added a commit that referenced this pull request Aug 19, 2019
@targos
2019-08-20, Version 12.9.0 (Current)
a87f0e1 
Notable changes:

* **crypto**:
  * Added an oaepHash option to asymmetric encryption which allows users to specify a hash function when using OAEP padding (Tobias Nießen) [#28335](#28335).
* **deps**:
  * Updated V8 to 7.6.303.29 (Michaël Zasso) [#28955](#28955).
    * Improves the performance of various APIs such as `JSON.parse` and methods
      called on frozen arrays.
    * Adds the [`Promise.allSettled`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/allSettled) method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0 (cjihrig) [#29070](#29070).
    * `UV_FS_O_FILEMAP` has been added for faster access to memory mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been added.
* **fs**:
  * Added `fs.writev` and `fs.writevSync` methods. They allow to write an array of `ArrayBufferView`s to a file descriptor (Anas Aboureada) [#25925](#25925).
* **http**:
  * Added three properties to `OutgoingMessage.prototype`: `writableObjectMode`, `writableLength` and `writableHighWaterMark` [#29018](#29018).
* **stream**:
  * Added an new property `writableEnded` to writable streams. Its value is set to `true` after `writable.end()` has been called. (Robert Nagy) [#28934](#28934).

PR-URL: #29210
cjihrig added a commit to cjihrig/node that referenced this pull request Aug 19, 2019
@cjihrig
fs: add writev() promises version
3273d0e 
nodejs#25925 added fs.writev()
and fs.writevSync(), but did not include a Promises based
equivalent. This commit adds the missing method.

Refs: nodejs#25925
PR-URL: nodejs#29186
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Aug 20, 2019
@cjihrig @targos
fs: validate writev fds consistently
4356836 
This commit updates the recently added writev methods
to validate file descriptors like the other fs methods do.

PR-URL: #29185
Refs: #25925
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Aug 20, 2019
@cjihrig @targos
fs: add writev() promises version
81f3eb5 
#25925 added fs.writev()
and fs.writevSync(), but did not include a Promises based
equivalent. This commit adds the missing method.

Refs: #25925
PR-URL: #29186
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Aug 20, 2019
@targos
2019-08-20, Version 12.9.0 (Current)
43d6619 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    #28335
* deps:
  * Updated V8 to 7.6.303.29. #28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. #29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. #25925
    https://github.com/nodejs/node/pull/29186/files
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    #29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    #28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    #28934

PR-URL: #29210
targos added a commit to targos/node that referenced this pull request Aug 20, 2019
@targos
2019-08-20, Version 12.9.0 (Current)
a9adfdb 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    nodejs#28335
* deps:
  * Updated V8 to 7.6.303.29. nodejs#28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. nodejs#29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. nodejs#25925
    https://github.com/nodejs/node/pull/29186/files
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    nodejs#29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    nodejs#28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    nodejs#28934

PR-URL: nodejs#29210
targos added a commit that referenced this pull request Aug 20, 2019
@targos
2019-08-20, Version 12.9.0 (Current)
e6872f5 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    #28335
* deps:
  * Updated V8 to 7.6.303.29. #28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. #29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. #25925
    #29186
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    #29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    #28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    #28934

PR-URL: #29210
targos added a commit that referenced this pull request Aug 20, 2019
@targos
2019-08-20, Version 12.9.0 (Current)
edc83a9 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    #28335
* deps:
  * Updated V8 to 7.6.303.29. #28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. #29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. #25925
    #29186
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    #29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    #28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    #28934

PR-URL: #29210
This was referenced Aug 20, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@bnoordhuis bnoordhuis bnoordhuis left review comments

@thefourtheye thefourtheye thefourtheye left review comments

@reqshark reqshark reqshark left review comments

@targos targos targos left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax 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. doc Issues and PRs related to the documentations. 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: expose scatter/gather syscalls writev() and readv()