fs: added fs.writev() which exposes syscalls writev() by AnasAboreeda · Pull Request #25925 · nodejs/node

AnasAboreeda · 2019-02-04T16:23:40Z

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

lib/fs.js

doc/api/fs.md

mscdex · 2019-02-04T16:35:05Z

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 · 2019-02-04T16:38:29Z

@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 · 2019-02-04T16:56:15Z

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 · 2019-02-04T17:37:01Z

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.

lib/fs.js

test/parallel/test-fs-writev.js

AnasAboreeda · 2019-02-06T12:16:40Z

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

addaleax

Nice work!

lib/fs.js

test/parallel/test-fs-writev.js

tools/doc/type-parser.js

doc/api/fs.md

jasnell · 2019-02-06T16:41:03Z

lib/fs.js

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

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

test/parallel/test-fs-writev.js

doc/api/fs.md

test/parallel/test-fs-writev.js

mscdex · 2019-02-07T07:12:25Z

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

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>

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

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>

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>

#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>

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

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

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

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 reviewed Feb 4, 2019

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

mscdex reviewed Feb 4, 2019

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

mscdex reviewed Feb 4, 2019

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

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

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

addaleax approved these changes Feb 6, 2019

View reviewed changes

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

AnasAboreeda force-pushed the expose-syscall-writev-to-fs branch from 1519909 to 9a39133 Compare February 6, 2019 15:31

jasnell reviewed Feb 6, 2019

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

jasnell reviewed Feb 6, 2019

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

jasnell reviewed Feb 6, 2019

View reviewed changes

mscdex reviewed Feb 6, 2019

View reviewed changes

test/parallel/test-fs-writev.js Outdated Show resolved Hide resolved

mscdex reviewed Feb 6, 2019

View reviewed changes

test/parallel/test-fs-writev.js Outdated Show resolved Hide resolved

AnasAboreeda force-pushed the expose-syscall-writev-to-fs branch from 2e3f895 to 137dbcb Compare February 6, 2019 20:07

mscdex reviewed Feb 7, 2019

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

mscdex reviewed Feb 7, 2019

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

mscdex reviewed Feb 7, 2019

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

mscdex reviewed Feb 7, 2019

View reviewed changes

test/parallel/test-fs-writev.js Outdated Show resolved Hide resolved

AnasAboreeda force-pushed the expose-syscall-writev-to-fs branch from 137dbcb to 35e8aa8 Compare February 7, 2019 06:37

mscdex reviewed Feb 7, 2019

View reviewed changes

test/parallel/test-fs-writev.js Outdated Show resolved Hide resolved

mscdex reviewed Feb 7, 2019

View reviewed changes

test/parallel/test-fs-writev.js Outdated Show resolved Hide resolved

targos mentioned this pull request Aug 19, 2019

v12.9.0 release proposal #29210

Merged

Uh oh!

Conversation

AnasAboreeda commented Feb 4, 2019

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mscdex commented Feb 4, 2019

Uh oh!

AnasAboreeda commented Feb 4, 2019

Uh oh!

mscdex commented Feb 4, 2019

Uh oh!

addaleax commented Feb 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AnasAboreeda commented Feb 6, 2019

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasnell Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

mscdex Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mscdex commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

mscdex commented Feb 7, 2019 •

edited

Loading