fs: added _writev (up to x50 faster writes) #2167

ronkorving · 2015-07-12T06:56:00Z

fs.WriteStream did not yet have a _writev method, so writing many buffers at once could not be optimized by libuv. This adds the function.

I've not yet benchmarked the difference before/after, as I'm not sure how to effectively benchmark disk I/O to be honest (because of disk cache, SSD vs. spinning disk (I'm using a mac fusion drive, that doesn't help)).
Update: benchmarks below

However, with this change:

There will be less system calls
There will be less jumping between JS land and native land

So I expect throughput to increase a little bit, while at the same time reducing CPU usage.

I didn't add tests, as this was already covered by existing tests (which failed during implementation, but now pass).

Warning: this is my first PR to io.js, so please be rigorous in review.

reqshark · 2015-07-12T08:02:21Z

we should see more efficient data transfer from any program that avoids redundant data copies between intermediate buffers and reduces the number of context switches between user space and kernel space

scatter/gather I/O! 👍

ronkorving · 2015-07-12T10:06:07Z

I noticed that with a very large amount of buffers, we hit a limit and writev (in libuv) fails with EINVAL (cc @bnoordhuis). This can be seen when running the fs benchmark.

Ideally, libuv would detect uv__fs_write getting too many buffers (through uv__getiovmax) and split up the writes into multiple writev calls.

ronkorving · 2015-07-12T10:51:34Z

After patching libuv (I'll send a PR) I got the problem to disappear.
Update: there already is a PR to address the issue: libuv/libuv#269
Update 2: the work has moved into PR libuv/libuv#448

Benchmark on OSX by running ./iojs benchmark/common.js fs write-stream-throughput

Before (master):

fs/write-stream-throughput.js dur=5 type=buf size=2: 0.09258
fs/write-stream-throughput.js dur=5 type=buf size=1024: 43.09845
fs/write-stream-throughput.js dur=5 type=buf size=65535: 268.53789
fs/write-stream-throughput.js dur=5 type=buf size=1048576: 253.07612
fs/write-stream-throughput.js dur=5 type=asc size=2: 0.09160
fs/write-stream-throughput.js dur=5 type=asc size=1024: 42.27883
fs/write-stream-throughput.js dur=5 type=asc size=65535: 253.94307
fs/write-stream-throughput.js dur=5 type=asc size=1048576: 272.43421
fs/write-stream-throughput.js dur=5 type=utf size=2: 0.09103
fs/write-stream-throughput.js dur=5 type=utf size=1024: 40.72314
fs/write-stream-throughput.js dur=5 type=utf size=65535: 290.43753
fs/write-stream-throughput.js dur=5 type=utf size=1048576: 254.93646

After:

fs/write-stream-throughput.js dur=5 type=buf size=2: 8.09139
fs/write-stream-throughput.js dur=5 type=buf size=1024: 239.52816
fs/write-stream-throughput.js dur=5 type=buf size=65535: 271.13331
fs/write-stream-throughput.js dur=5 type=buf size=1048576: 272.94912
fs/write-stream-throughput.js dur=5 type=asc size=2: 2.95558
fs/write-stream-throughput.js dur=5 type=asc size=1024: 210.82502
fs/write-stream-throughput.js dur=5 type=asc size=65535: 234.68711
fs/write-stream-throughput.js dur=5 type=asc size=1048576: 294.56500
fs/write-stream-throughput.js dur=5 type=utf size=2: 2.74160
fs/write-stream-throughput.js dur=5 type=utf size=1024: 184.46578
fs/write-stream-throughput.js dur=5 type=utf size=65535: 284.54823
fs/write-stream-throughput.js dur=5 type=utf size=1048576: 279.93069

The impact is (not unexpectedly) much bigger with many buffers (ie: the case where buffer size is small). I must note though that being on a hybrid SSD/HD (fusion drive) makes this not the most scientifically sound benchmark.

brendanashworth · 2015-07-16T19:02:43Z

src/node_file.cc

+static void WriteBuffers(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+
+  if (!args[0]->IsInt32())


Because this binding isn't accessible to the end user, we typically leave out the well-worded error messages and instead leave in CHECK()s, like this:

CHECK(args[0]->IsInt32()); CHECK(args[1]->IsArray());

Here is an example.

Right, I kinda went with what I saw in WriteBuffer, which is located right above my WriteBuffers function. It too has a test like:

if (!args[0]->IsInt32()) return env->ThrowTypeError("First argument must be file descriptor");

I just followed that.

brendanashworth · 2015-07-16T19:10:56Z

Thanks for taking the time to put this together! I left a few small nits. The benchmark does seem to be looking good right now, but we do have to wait for that libuv patch. Hopefully that will work out well and make its way into the code base.

ronkorving · 2015-07-17T01:05:19Z

No worries! I've fixed those few little things (thanks for the review!), so now all attention goes back to libuv. If the original author of that PR does not respond before Monday or so, I will dive in and fix it myself.

thefourtheye · 2015-07-17T04:32:41Z

lib/fs.js

+  var req = new FSReqWrap();
+  req.oncomplete = wrapper;
+  binding.writeBuffers(fd, chunks, position, req);
+};


; is not necessary for function declarations

Nice catch, I wonder why the linter didn't catch that.

@ronkorving it is because the related eslint rule (no-extra-semi) is not enabled.
I have a working branch to fix that. @silverwind what do you think about it ?

I've been meaning to look at enabling semicolon related rules, but didn't find time yet. Go ahead and do a PR if you like. One commit to enable the rule(s), another to fix any issues.

@ronkorving Can you remove this ;?

trevnorris · 2015-07-22T18:03:43Z

src/node_file.cc

+    const char* buf = Buffer::Data(chunk);
+    size_t len = Buffer::Length(chunk);
+
+    iovs[i] = uv_buf_init(const_cast<char *>(buf), len);


style nit: <char*>

note: I see it's done that way in Read(), but that style originated with Ryan way back in 2010. The style has changed and we just propagate w/ new code.

Understood.

trevnorris · 2015-07-22T18:16:18Z

Will need docs and tests. Also I would recommend exposing this via fs.writev. Or hooking into fs.write to detect if an array is passed. This would be very helpful to access directly.

Some comments and style nits listed. Looking good.

ronkorving · 2015-07-28T05:02:26Z

I agree it would be interesting to expose this through fs.write or fs.writev, but do you think that should be part of this PR? I don't mind iterating in multiple smaller PRs, especially since I expect some debate over API on that one, and I see little reason for that feature to block this PR. Just my thoughts, but what do you think @trevnorris ?

ronkorving · 2015-09-14T14:42:26Z

I quote:

fs: implemented WriteStream#writev
Streams with writev allow many buffers to be pushed to underlying
OS APIs in one batch, in this case improving write throughput by
an order of magnitude. This is especially noticable when writing
many (small) buffers.

trevnorris · 2015-09-14T22:41:55Z

@ronkorving WTF. Mobile output wasn't showing the extended comment. Thanks. Reads well.

Streams with writev allow many buffers to be pushed to underlying OS APIs in one batch, in this case improving write throughput by an order of magnitude. This is especially noticeable when writing many (small) buffers. PR-URL: #2167 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

trevnorris · 2015-09-14T22:52:16Z

Thanks. Landed on 05d30d5 and removed extra semicolon.

Streams with writev allow many buffers to be pushed to underlying OS APIs in one batch, in this case improving write throughput by an order of magnitude. This is especially noticeable when writing many (small) buffers. PR-URL: #2167 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

rvagg · 2015-09-15T06:51:56Z

This is semver-minor right? tagging it as so, @trevnorris @ronkorving please correct me if I'm wrong

rvagg · 2015-09-15T06:52:14Z

@Fishrock123 if you've cherry-picked this then the first bump of 4.x will have to be 4.1.0

ronkorving · 2015-09-15T06:54:46Z

I'm fine either way, but how is this not patch? It introduces internal-only API for improved performance that is not exposed in any way to the outside world. Is patch only allowed to be bugfixes?

rvagg · 2015-09-15T07:20:35Z

Hmm, perhaps you're right, I saw env->SetMethod(target, "writeBuffers", WriteBuffers); and jumped to the conclusion. This is only exposed via process.binding('fs').writeBuffers right? Are we happy to say that everything in process.binding is OOB for semver- labels and it doesn't matter what we do in there that we don't have to version for it? Thoughts anyone else?

Fishrock123 · 2015-09-15T17:21:59Z

cc @nodejs/tsc, is this minor or patch?

I'm currently leaning toward minor. If our first post 4.0.0 is a minor it doesn't bother me. Such is the way of SemVer.

trevnorris · 2015-09-15T17:22:08Z

Are we happy to say that everything in process.binding is OOB for semver

I really hope so, or else we've all been breaking the rules. :P

rvagg · 2015-09-16T02:28:54Z

Here's my take on it: this PR adds two publicly accessible things, process.binding('fs').writeBuffers and fs.WriteStream.prototype._writev. Regardless of the fact that it's on process.bindings or in an underscore method, people will use it, perhaps even readable-stream could benefit from using it directly in some way (that's not likely but consider it an example).

Our approach to semver shouldn't be to try and pull things down to the minimum but just the opposite. We should be using the maximum part of semver that could possibly make sense for a change. If something might break someone's workflow then it should be semver-major, if something we add might be used by end-users then it should be a semver-minor. semver-patch should only be used reserved for cases where we have a high degree of certainty that it's not going to break for someone and that it's not adding new surface area to our exposed API.

So I'm still +1 on this being semver-minor even if there's no documentation added for what's been added.

Also, thanks @ronkorving, this a great addition!

ronkorving · 2015-09-16T02:32:48Z

You're welcome :) I'm just happy to see it land, no matter the version (I'll leave that up to your judgement).

Notable changes: * buffer: - Buffers are now created in JavaScript, rather than C++. This increases the speed of buffer creation (Trevor Norris) nodejs#2866. - `Buffer#slice()` now uses `Uint8Array#subarray()` internally, increasing `slice()` performance (Karl Skomski) nodejs#2777. * fs: - `fs.utimes()` now properly converts numeric strings, `NaN`, and `Infinity` (Yazhong Liu) nodejs#2387. - `fs.WriteStream` now implements `_writev`, allowing for super-fast bulk writes (Ron Korving) nodejs#2167. * http: Fixed an issue with certain `write()` sizes causing errors when using `http.request()` (Fedor Indutny) nodejs#2824. * npm: Upgrade to version 2.14.3, see https://github.com/npm/npm/releases/tag/v2.14.3 for more details (Kat Marchán) nodejs#2822. * src: V8 cpu profiling no longer erroneously shows idle time (Oleksandr Chekhovskyi) nodejs#2324. * v8: Lateral upgrade to 4.5.103.33 from 4.5.103.30, contains minor fixes (Ali Ijaz Sheikh) nodejs#2870. - This fixes a previously known bug where some computed object shorthand properties did not work correctly (nodejs#2507). Refs: nodejs#2844 PR-URL: nodejs#2889

Notable changes: * buffer: - Buffers are now created in JavaScript, rather than C++. This increases the speed of buffer creation (Trevor Norris) #2866. - `Buffer#slice()` now uses `Uint8Array#subarray()` internally, increasing `slice()` performance (Karl Skomski) #2777. * fs: - `fs.utimes()` now properly converts numeric strings, `NaN`, and `Infinity` (Yazhong Liu) #2387. - `fs.WriteStream` now implements `_writev`, allowing for super-fast bulk writes (Ron Korving) #2167. * http: Fixed an issue with certain `write()` sizes causing errors when using `http.request()` (Fedor Indutny) #2824. * npm: Upgrade to version 2.14.3, see https://github.com/npm/npm/releases/tag/v2.14.3 for more details (Kat Marchán) #2822. * src: V8 cpu profiling no longer erroneously shows idle time (Oleksandr Chekhovskyi) #2324. * v8: Lateral upgrade to 4.5.103.33 from 4.5.103.30, contains minor fixes (Ali Ijaz Sheikh) #2870. - This fixes a previously known bug where some computed object shorthand properties did not work correctly (#2507). Refs: #2844 PR-URL: #2889

ronkorving mentioned this pull request Jul 12, 2015

streams: add cork option to pipe #2020

Closed

brendanashworth 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 Jul 12, 2015

ronkorving force-pushed the fs-writev branch from eb10c94 to cf409a4 Compare July 12, 2015 12:48

ronkorving changed the title ~~fs: added _writev for faster writes in the case of many buffers~~ fs: added _writev (up to x100 faster writes) Jul 13, 2015

ronkorving mentioned this pull request Jul 14, 2015

unix: allow nbufs>IOV_MAX in uv_fs_read and uv_fs_write libuv/libuv#269

Closed

brendanashworth reviewed Jul 16, 2015
View reviewed changes

Fishrock123 added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Jul 16, 2015

thefourtheye reviewed Jul 17, 2015
View reviewed changes

ronkorving mentioned this pull request Jul 20, 2015

unix: allow nbufs>IOV_MAX in uv_fs_read and uv_fs_write (replaces #269) libuv/libuv#448

Closed

trevnorris reviewed Jul 22, 2015
View reviewed changes

indutny force-pushed the master branch from bffb204 to eb35968 Compare July 22, 2015 21:21

trevnorris closed this Sep 14, 2015

trevnorris added the land-on-v4.x label Sep 14, 2015

rvagg added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 15, 2015

rvagg mentioned this pull request Sep 15, 2015

Release proposal: v3.3.1 #2698

Merged

ronkorving deleted the fs-writev branch September 17, 2015 05:51

ronkorving mentioned this pull request Sep 18, 2015

fs: speed up fs.WriteStream#_write #2944

Closed

rvagg mentioned this pull request Sep 22, 2015

Release proposal: v4.1.1 #2995

Merged

jasnell added the lts-landed-on-v4.x label Oct 7, 2015

MylesBorins removed the land-on-v4.x label Nov 16, 2015

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade prismjs from 1.17.1 to 1.19.0 O330oei/node#14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: added _writev (up to x50 faster writes) #2167

fs: added _writev (up to x50 faster writes) #2167

ronkorving commented Jul 12, 2015

reqshark commented Jul 12, 2015

ronkorving commented Jul 12, 2015

ronkorving commented Jul 12, 2015

brendanashworth Jul 16, 2015

ronkorving Jul 17, 2015

brendanashworth commented Jul 16, 2015

ronkorving commented Jul 17, 2015

thefourtheye Jul 17, 2015

ronkorving Jul 17, 2015

targos Jul 17, 2015

silverwind Jul 17, 2015

thefourtheye Sep 2, 2015

trevnorris Jul 22, 2015

trevnorris Jul 22, 2015

ronkorving Jul 27, 2015

trevnorris commented Jul 22, 2015

ronkorving commented Jul 28, 2015

ronkorving commented Sep 14, 2015

trevnorris commented Sep 14, 2015

trevnorris commented Sep 14, 2015

rvagg commented Sep 15, 2015

rvagg commented Sep 15, 2015

ronkorving commented Sep 15, 2015

rvagg commented Sep 15, 2015

Fishrock123 commented Sep 15, 2015

trevnorris commented Sep 15, 2015

rvagg commented Sep 16, 2015

ronkorving commented Sep 16, 2015

fs: added _writev (up to x50 faster writes) #2167

fs: added _writev (up to x50 faster writes) #2167

Conversation

ronkorving commented Jul 12, 2015

reqshark commented Jul 12, 2015

ronkorving commented Jul 12, 2015

ronkorving commented Jul 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanashworth commented Jul 16, 2015

ronkorving commented Jul 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Jul 22, 2015

ronkorving commented Jul 28, 2015

ronkorving commented Sep 14, 2015

trevnorris commented Sep 14, 2015

trevnorris commented Sep 14, 2015

rvagg commented Sep 15, 2015

rvagg commented Sep 15, 2015

ronkorving commented Sep 15, 2015

rvagg commented Sep 15, 2015

Fishrock123 commented Sep 15, 2015

trevnorris commented Sep 15, 2015

rvagg commented Sep 16, 2015

ronkorving commented Sep 16, 2015