src: improve StreamBase read throughput #23797

addaleax · 2018-10-21T08:44:14Z

Improve performance by providing JS with the raw ingredients
for the read data, i.e. an ArrayBuffer + offset + length
fields, instead of creating Buffer instances in C++ land.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Improve performance by providing JS with the raw ingridients for the read data, i.e. an `ArrayBuffer` + offset + length fields, instead of creating `Buffer` instances in C++ land.

nodejs-github-bot · 2018-10-21T08:44:15Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1324/pipeline

addaleax · 2018-10-21T08:45:48Z

~~CI: https://ci.nodejs.org/job/node-test-pull-request/18023/~~
CI: https://ci.nodejs.org/job/node-test-pull-request/18025/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/248/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1595/

TimothyGu

Are there benchmark results available?

TimothyGu · 2018-10-21T21:24:10Z

src/stream_base.cc

  Environment* env = env_;

+#ifdef DEBUG
+  CHECK_EQ(static_cast<int32_t>(nread), nread);
+  CHECK_EQ(static_cast<int32_t>(offset), offset);


Maybe also assert that offset is 0 and nread is less than 0 if ab is empty?

IIUC this will also allow us to remove
// TODO(bnoordhuis) Check that nread > 0.
https://github.com/nodejs/node/pull/23797/files#diff-f45eb699237c2e38dc9b49b588933c11R497.
in channel.onread

Edit: that is if we add a reverse check of ab non-empty -> nread > 0

I’m not sure we can guarantee nread > 0, but nread >= 0 should make sense.

mscdex · 2018-10-22T10:36:55Z

New benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/249/

mscdex · 2018-10-22T16:19:07Z

It looks like benchmark/net/tcp-raw-c2s.js needs to be fixed.

addaleax · 2018-10-23T03:18:01Z

It looks like benchmark/net/tcp-raw-c2s.js needs to be fixed.

Done!

With more iterations this time: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/250/ (edit: aborted, took too long. sorry… the other benchmarks results are pretty good, though.)

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/18065/

mscdex · 2018-10-23T20:44:13Z

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/251/console

It looks like benchmark/net/tcp-raw-pipe.js needs to be updated (further):

buffer.js:208
    throw new ERR_INVALID_ARG_TYPE('value', 'not number', value);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "value" argument must not be of type number. Received type number
    at Function.from (buffer.js:208:11)
    at TCP.clientHandle.onread (/home/iojs/build/workspace/benchmark-node-micro-benchmarks/benchmarking/experimental/benchmarks/community-benchmark/node/benchmark/net/tcp-raw-pipe.js:54:55)

addaleax · 2018-10-24T06:53:23Z

@mscdex That’s happening because the modified benchmark and the previous version of it are incompatible, because they expect different function signatures… I’m going to open a PR to remove those raw benchmarks right after landing this anyway.

addaleax · 2018-10-24T06:55:45Z

Resume CI (sigh): https://ci.nodejs.org/job/node-test-pull-request/18097/

addaleax · 2018-10-24T07:58:20Z

Landed in 1365f65

Improve performance by providing JS with the raw ingridients for the read data, i.e. an `ArrayBuffer` + offset + length fields, instead of creating `Buffer` instances in C++ land. PR-URL: #23797 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

p3x-robot · 2018-11-19T12:23:59Z

Hello!
We have a change that fails in SPDY since Node v11.1.0 (spdy-http2/node-spdy#350 (comment))

Could you tell us what the actual is changing? Is it only the signature of those files and nothing else?

addaleax · 2018-11-19T12:37:03Z

@p3x-robot I’ve commented on the linked issue, hope that’s helpful

p3x-robot · 2018-11-19T12:49:00Z

found it, thanks very much!

MylesBorins · 2018-11-26T20:33:50Z

Should this be backported to v8.x-staging or v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

src: improve StreamBase read throughput

744d88c

Improve performance by providing JS with the raw ingridients for the read data, i.e. an `ArrayBuffer` + offset + length fields, instead of creating `Buffer` instances in C++ land.

nodejs-github-bot added the lib / src label Oct 21, 2018

addaleax added net performance labels Oct 21, 2018

addaleax added this to To do in StreamBase refactor via automation Oct 21, 2018

fixup! src: improve StreamBase read throughput

6023661

TimothyGu approved these changes Oct 21, 2018

View changes

jasnell approved these changes Oct 21, 2018

View changes

fixup! src: improve StreamBase read throughput

18069e4

addaleax force-pushed the net-throughput branch from 2350bd1 to 18069e4 Compare Oct 23, 2018

fixup! src: improve StreamBase read throughput

f829b61

addaleax added the author ready label Oct 24, 2018

addaleax closed this Oct 24, 2018

StreamBase refactor automation moved this from To do to Done Oct 24, 2018

addaleax deleted the net-throughput branch Oct 24, 2018

targos mentioned this pull request Oct 27, 2018

Release proposal: v11.1.0 #23922

Merged

beevelop mentioned this pull request Nov 2, 2018

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#245

Closed

beevelop mentioned this pull request Nov 2, 2018

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#246

Closed

beevelop mentioned this pull request Nov 2, 2018

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#247

Closed

beevelop mentioned this pull request Nov 2, 2018

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#248

Closed

p3x-robot mentioned this pull request Nov 19, 2018

SPDY fails in node >= 11.1.0 spdy-http2/node-spdy#350

Closed

MylesBorins added lts-watch-v8.x labels Nov 26, 2018

MylesBorins added backport-requested-v8.x and removed lts-watch-v10.x labels Nov 26, 2018

nodejs / node Public

src: improve StreamBase read throughput #23797

src: improve StreamBase read throughput #23797

addaleax commented Oct 21, 2018 •

edited

nodejs-github-bot commented Oct 21, 2018

addaleax commented Oct 21, 2018 •

edited

TimothyGu left a comment

TimothyGu Oct 21, 2018

lundibundi Oct 22, 2018 •

edited

addaleax Oct 23, 2018

mscdex commented Oct 22, 2018

mscdex commented Oct 22, 2018

addaleax commented Oct 23, 2018 •

edited

mscdex commented Oct 23, 2018 •

edited

addaleax commented Oct 24, 2018

addaleax commented Oct 24, 2018

addaleax commented Oct 24, 2018

p3x-robot commented Nov 19, 2018

addaleax commented Nov 19, 2018

p3x-robot commented Nov 19, 2018

MylesBorins commented Nov 26, 2018

nodejs / node Public

src: improve StreamBase read throughput #23797

src: improve StreamBase read throughput #23797

Conversation

addaleax commented Oct 21, 2018 • edited

Checklist

nodejs-github-bot commented Oct 21, 2018

addaleax commented Oct 21, 2018 • edited

TimothyGu left a comment

TimothyGu Oct 21, 2018

Choose a reason for hiding this comment

lundibundi Oct 22, 2018 • edited

Choose a reason for hiding this comment

addaleax Oct 23, 2018

Choose a reason for hiding this comment

mscdex commented Oct 22, 2018

mscdex commented Oct 22, 2018

addaleax commented Oct 23, 2018 • edited

mscdex commented Oct 23, 2018 • edited

addaleax commented Oct 24, 2018

addaleax commented Oct 24, 2018

addaleax commented Oct 24, 2018

p3x-robot commented Nov 19, 2018

addaleax commented Nov 19, 2018

p3x-robot commented Nov 19, 2018

MylesBorins commented Nov 26, 2018

addaleax commented Oct 21, 2018 •

edited

addaleax commented Oct 21, 2018 •

edited

lundibundi Oct 22, 2018 •

edited

addaleax commented Oct 23, 2018 •

edited

mscdex commented Oct 23, 2018 •

edited