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

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

Closed
wants to merge 1 commit into from

Conversation

@ronkorving
Copy link
Contributor

ronkorving commented Jul 12, 2015

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

This comment has been minimized.

Copy link

reqshark commented Jul 12, 2015

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

This comment has been minimized.

Copy link
Contributor Author

ronkorving commented Jul 12, 2015

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

This comment has been minimized.

Copy link
Contributor Author

ronkorving commented Jul 12, 2015

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.

@ronkorving ronkorving force-pushed the ronkorving:fs-writev branch Jul 12, 2015
@ronkorving 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
@brendanashworth
brendanashworth reviewed Jul 16, 2015
View changes
src/node_file.cc Outdated
static void WriteBuffers(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

if (!args[0]->IsInt32())

This comment has been minimized.

Copy link
@brendanashworth

brendanashworth Jul 16, 2015

Contributor

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.

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 17, 2015

Author Contributor

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
brendanashworth reviewed Jul 16, 2015
View changes
src/node_file.cc Outdated

uint32_t chunkCount = chunks->Length();

uv_buf_t * iovs = new uv_buf_t[chunkCount];

This comment has been minimized.

Copy link
@brendanashworth

brendanashworth Jul 16, 2015

Contributor

Sorry for the tiny nit, but this should be uv_buf_t* iovs.

@brendanashworth

This comment has been minimized.

Copy link
Contributor

brendanashworth commented Jul 16, 2015

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.

@Fishrock123 Fishrock123 added the libuv label Jul 16, 2015
@ronkorving

This comment has been minimized.

Copy link
Contributor Author

ronkorving commented Jul 17, 2015

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
thefourtheye reviewed Jul 17, 2015
View changes
lib/fs.js Outdated
var req = new FSReqWrap();
req.oncomplete = wrapper;
binding.writeBuffers(fd, chunks, position, req);
};

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jul 17, 2015

Contributor

; is not necessary for function declarations

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 17, 2015

Author Contributor

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

This comment has been minimized.

Copy link
@targos

targos Jul 17, 2015

Member

@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 ?

This comment has been minimized.

Copy link
@silverwind

silverwind Jul 17, 2015

Contributor

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.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Sep 2, 2015

Contributor

@ronkorving Can you remove this ;?

});

if (this.pos !== undefined)
this.pos += size;

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jul 17, 2015

Contributor

What if the write fails? Is changing pos okay even in that case?

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 17, 2015

Author Contributor

I didn't actually change this logic. It was already like this. However, I think it's a safe operation. The most important thing of using pos is that the next call starts at the right offset. If the write fails, there will be an error and the stream will be destroyed anyway.

// bytesWritten = writev(fd, chunks, position, callback)
// 0 fd integer. file descriptor
// 1 chunks array of buffers to write
// 2 position if integer, position to write at in the file.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jul 17, 2015

Contributor

Can you change at in?

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 17, 2015

Author Contributor

Could, but it is a literal copy of the comment above writeBuffer which I didn't author. Would you still want me to fix it, and if so, in both places?

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 28, 2015

Author Contributor

I don't really see how the English in this line is incorrect btw (disclaimer: not a native English speaker).


int fd = args[0]->Int32Value();
Local<Array> chunks = args[1].As<Array>();
int64_t pos = GET_OFFSET(args[2]);

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jul 17, 2015

Contributor

As per the comment above this function, this could be null as well right?

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 17, 2015

Author Contributor

Absolutely. In fact, that's the common case as far as I can tell. GET_OFFSET returns -1 in that case.

@thefourtheye
thefourtheye reviewed Jul 17, 2015
View changes
src/node_file.cc Outdated
args.GetReturnValue().Set(SYNC_RESULT);
}


This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jul 17, 2015

Contributor

Extra newline

this.pos += size;
};


This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jul 17, 2015

Contributor

Extra newline

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 17, 2015

Author Contributor

The rule is 1?

This comment has been minimized.

Copy link
@brendanashworth

brendanashworth Jul 17, 2015

Contributor

Not quite, usually two spaces are used to separate unrelated function declarations. It is 3+ where the linter complains.

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 21, 2015

Author Contributor

So the current 2 is fine then?

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Jul 21, 2015

Member

Correct, 2 is fine.

Local<Value> req = args[3];

uint32_t chunkCount = chunks->Length();

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jul 17, 2015

Contributor

Extra newlines between declarations

This comment has been minimized.

Copy link
@brendanashworth

brendanashworth Jul 17, 2015

Contributor

I think it is more readable, personally.

@@ -1867,6 +1867,49 @@ WriteStream.prototype._write = function(data, encoding, cb) {
};


function writev(fd, chunks, position, callback) {
function wrapper(err, written) {
// Retain a reference to chunks so that they can't be GC'ed too soon.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Jul 17, 2015

Contributor

Can you please explain why this is necessary, if you don't mind me asking?

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 17, 2015

Author Contributor

I'm following the other examples in fs.js that have this pattern. I did not invent this to be honest.
My guess is that not having the chunks (that contain buffers) in this closure would GC and free the buffers before libuv has a chance to access and use them.

This comment has been minimized.

Copy link
@brendanashworth

brendanashworth Jul 17, 2015

Contributor

Yes, that is correct (afaik). Buffers allocate their own memory and delimit deallocating to v8, which doesn't take into account the event loop. Relevant: f9b7714

This comment has been minimized.

Copy link
@trevnorris

trevnorris Jul 22, 2015

Contributor

Unfortunately I am a culprit of propagating this practice. Best I can tell it originated with e7604b1. I then furthered what had been done in commits like 7ca77ea. In retrospect I should have changed this when I introduced FSReqWrap() in 819690f.

Don't worry about changing it for this PR though. There should be a single clean commit that addresses just this issue. I'll put that change on my list.

@trevnorris
trevnorris reviewed Jul 22, 2015
View changes
src/node_file.cc Outdated
const char* buf = Buffer::Data(chunk);
size_t len = Buffer::Length(chunk);

iovs[i] = uv_buf_init(const_cast<char *>(buf), len);

This comment has been minimized.

Copy link
@trevnorris

trevnorris Jul 22, 2015

Contributor

style nit: <char*>

This comment has been minimized.

Copy link
@trevnorris

trevnorris Jul 22, 2015

Contributor

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.

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 27, 2015

Author Contributor

Understood.

@trevnorris
trevnorris reviewed Jul 22, 2015
View changes
src/node_file.cc Outdated
}

if (req->IsObject()) {
ASYNC_CALL(write, req, fd, iovs, chunkCount, pos)

This comment has been minimized.

Copy link
@trevnorris

trevnorris Jul 22, 2015

Contributor

Done see req used anywhere else. You can pass args[3] here directly.

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 27, 2015

Author Contributor

It's used to check if it's an object (line 924), then passed (line 925). Sure, I could remove it, but how would that help? I doubt the compiler isn't smart enough to optimize this one way or another if you think this is inefficient.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Jul 27, 2015

Contributor

fail on my part. don't worry about it.

@trevnorris
trevnorris reviewed Jul 22, 2015
View changes
src/node_file.cc Outdated

uint32_t chunkCount = chunks->Length();

uv_buf_t* iovs = new uv_buf_t[chunkCount];

This comment has been minimized.

Copy link
@trevnorris

trevnorris Jul 22, 2015

Contributor

Small optimization note. It's unlikely that more than 1024 chunks will be passed, so you could stack allocate a fixed amount and only allocate if number of chunks exceed that.

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 27, 2015

Author Contributor

Is that safe given the ASYNC_CALL below? That pointer on the stack won't outlast its stack frame?

This comment has been minimized.

Copy link
@trevnorris

trevnorris Jul 27, 2015

Contributor

I believe libuv makes a copy of all the uv_buf_t passed.

/cc @bnoordhuis

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Jul 27, 2015

Member

That's correct.

@trevnorris
trevnorris reviewed Jul 22, 2015
View changes
src/node_file.cc Outdated
return env->ThrowTypeError("Array elements all need to be buffers");
}

const char* buf = Buffer::Data(chunk);

This comment has been minimized.

Copy link
@trevnorris

trevnorris Jul 22, 2015

Contributor

Not sure why you'd use const char* just to const_cast<> it away a few lines down. Buffer::Data() only returns a char*.

This comment has been minimized.

Copy link
@ronkorving

ronkorving Jul 28, 2015

Author Contributor

Nice catch

@trevnorris

This comment has been minimized.

Copy link
Contributor

trevnorris commented Jul 22, 2015

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.

@indutny indutny force-pushed the nodejs:master branch to eb35968 Jul 22, 2015
@ronkorving

This comment has been minimized.

Copy link
Contributor Author

ronkorving commented Jul 28, 2015

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

This comment has been minimized.

Copy link
Contributor Author

ronkorving commented Sep 14, 2015

@ronkorving

This comment has been minimized.

Copy link
Contributor Author

ronkorving commented Sep 14, 2015

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

This comment has been minimized.

Copy link
Contributor

trevnorris commented Sep 14, 2015

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

trevnorris added a commit that referenced this pull request Sep 14, 2015
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

This comment has been minimized.

Copy link
Contributor

trevnorris commented Sep 14, 2015

Thanks. Landed on 05d30d5 and removed extra semicolon.

@trevnorris trevnorris closed this Sep 14, 2015
Fishrock123 added a commit that referenced this pull request Sep 15, 2015
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 rvagg added the semver-minor label Sep 15, 2015
@rvagg

This comment has been minimized.

Copy link
Member

rvagg commented Sep 15, 2015

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

@rvagg

This comment has been minimized.

Copy link
Member

rvagg commented Sep 15, 2015

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

@ronkorving

This comment has been minimized.

Copy link
Contributor Author

ronkorving commented Sep 15, 2015

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

This comment has been minimized.

Copy link
Member

rvagg commented Sep 15, 2015

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?

@rvagg rvagg referenced this pull request Sep 15, 2015
@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Sep 15, 2015

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

This comment has been minimized.

Copy link
Contributor

trevnorris commented Sep 15, 2015

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

This comment has been minimized.

Copy link
Member

rvagg commented Sep 16, 2015

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

This comment has been minimized.

Copy link
Contributor Author

ronkorving commented Sep 16, 2015

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

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 17, 2015
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
Fishrock123 added a commit that referenced this pull request Sep 17, 2015
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 ronkorving deleted the ronkorving:fs-writev branch Sep 17, 2015
@rvagg rvagg referenced this pull request Sep 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.