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

lib,src: improve writev() performance for Buffers #13187

Merged
merged 1 commit into from May 26, 2017

Conversation

Projects
None yet
5 participants
@mscdex
Contributor

mscdex commented May 24, 2017

It's pretty common to write only Buffers to a socket, so we can optimize for that case in a couple of ways whenever we know we have all Buffers queued up for writev():

  • Reuse the existing array of chunks
  • Try to write the Buffers (or at least as much as possible) immediately without having to create a WriteWrap instance first. This optimization is one that is already being done when write()ing a single Buffer (or a single string for that matter) to a socket.

Benchmark results:

                                                improvement confidence      p.value
 net/net-c2s-cork.js dur=5 type="buf" len=1024     32.36 %        *** 3.042522e-23
 net/net-c2s-cork.js dur=5 type="buf" len=128      27.74 %        *** 5.759989e-59
 net/net-c2s-cork.js dur=5 type="buf" len=16       30.72 %        *** 1.594866e-57
 net/net-c2s-cork.js dur=5 type="buf" len=32       31.18 %        *** 8.568983e-62
 net/net-c2s-cork.js dur=5 type="buf" len=4        24.99 %        *** 2.220787e-64
 net/net-c2s-cork.js dur=5 type="buf" len=512      29.04 %        *** 3.058048e-32
 net/net-c2s-cork.js dur=5 type="buf" len=64       29.34 %        *** 5.167685e-50
 net/net-c2s-cork.js dur=5 type="buf" len=8        11.35 %        *** 4.258937e-43

CI: https://ci.nodejs.org/job/node-test-pull-request/8271/
CI for FIPS (since it failed initially): https://ci.nodejs.org/job/node-test-commit-linux-fips/8624/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • net
  • stream
while (entry) {
buffer[count] = entry;
if (!entry.isBuf)

This comment has been minimized.

@benjamingr

benjamingr May 24, 2017

Member

I'd probably do allBuffers = allBuffers && entry.isBuf

This comment has been minimized.

@mscdex

mscdex May 24, 2017

Contributor

I tried something similar (continuing the rest of the loop without any conditional) and did not see any difference in performance.

src/stream_base.cc Outdated
@@ -100,94 +100,118 @@ int StreamBase::Writev(const FunctionCallbackInfo<Value>& args) {
Local<Object> req_wrap_obj = args[0].As<Object>();
Local<Array> chunks = args[1].As<Array>();
bool all_buffers = args[2]->BooleanValue();

This comment has been minimized.

@addaleax

addaleax May 24, 2017

Member

Can you use the overload that returns a Maybe<bool>, or alternatively use IsTrue() instead?

This comment has been minimized.

@mscdex

mscdex May 24, 2017

Contributor

Changed.

if (all_buffers)
count = chunks->Length();
else
count = chunks->Length() >> 1;

This comment has been minimized.

@addaleax

addaleax May 24, 2017

Member

nit: I’d really just write / 2, it’s clearer and the resulting code will be the same

This comment has been minimized.

@mscdex

mscdex May 24, 2017

Contributor

That's copied from before these changes. It's also similar to what was already being used in js.

This comment has been minimized.

@addaleax

addaleax May 24, 2017

Member

Yeah, but in JS that has a different meaning. ¯\_(ツ)_/¯ If you want to keep it it’s fine.

src/stream_base.cc Outdated
wrap = GetAsyncWrap();
// NOTE: All tests show that GetAsyncWrap() never returns nullptr here. If it
// can then replace the CHECK_NE() with if (wrap != nullptr).
CHECK_NE(wrap, nullptr);

This comment has been minimized.

@addaleax

addaleax May 24, 2017

Member

Heads up, this is going to conflict with #13174 which removes the comment (because GetAsyncWrap() does actually never return a null pointer), so feel free to remove it here as well

This comment has been minimized.

@mscdex

mscdex May 24, 2017

Contributor

I'll just wait until that lands first and I'll rebase.

This comment has been minimized.

@addaleax

addaleax May 25, 2017

Member

@mscdex Landed, you can rebase now

This comment has been minimized.

@mscdex

mscdex May 25, 2017

Contributor

Done.

size_t offset = 0;
for (size_t i = 0; i < count; i++) {
Local<Value> chunk = chunks->Get(i * 2);
if (Buffer::HasInstance(chunk))

This comment has been minimized.

@addaleax

addaleax May 24, 2017

Member

nit: chunk->IsUint8Array()

This comment has been minimized.

@mscdex

mscdex May 24, 2017

Contributor

This was also copied from before these changes. I think changing it to check for a more general type is outside the scope of this PR.

This comment has been minimized.

@addaleax

addaleax May 24, 2017

Member

I think changing it to check for a more general type is outside the scope of this PR.

It’s literally the same thing, just inlined. :)

This comment has been minimized.

@mscdex

mscdex May 24, 2017

Contributor

Actually HasInstance() in master and v8.x use IsArrayBufferView() while v7.x and v6.x use IsUint8Array(). Ideally this could be backported more easily if we kept HasInstance()?

This comment has been minimized.

@addaleax

addaleax May 24, 2017

Member

Oh, right, we made that switch. Idc, you can also keep Buffer::HasInstance.

lib,src: improve writev() performance for Buffers
PR-URL: #13187
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@mscdex mscdex merged commit 01a1022 into nodejs:master May 26, 2017

@mscdex mscdex deleted the mscdex:net-writev-perf branch May 26, 2017

jasnell added a commit that referenced this pull request May 28, 2017

lib,src: improve writev() performance for Buffers
PR-URL: #13187
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Jul 17, 2017

Should we consider this for LTS? if so it needs to bake for a bit. Please change labels as appropriate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment