src: improve StreamBase read throughput by addaleax · Pull Request #23797 · nodejs/node

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:22:14Z

src/stream_base.cc


+#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 Issues and PRs related to general changes in the lib or src directory. label Oct 21, 2018

addaleax added net Issues and PRs related to the net subsystem. performance Issues and PRs related to the performance of Node.js. labels Oct 21, 2018

fixup! src: improve StreamBase read throughput

6023661

TimothyGu approved these changes Oct 21, 2018

View reviewed changes

jasnell approved these changes Oct 21, 2018

View reviewed changes

fixup! src: improve StreamBase read throughput

18069e4

addaleax force-pushed the net-throughput branch from 2350bd1 to 18069e4 Compare October 23, 2018 03:17

fixup! src: improve StreamBase read throughput

f829b61

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 24, 2018

addaleax closed this Oct 24, 2018

addaleax deleted the net-throughput branch October 24, 2018 07:58

targos mentioned this pull request Oct 27, 2018

Release proposal: v11.1.0 #23922

Merged

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

Uh oh!

Conversation

addaleax commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Oct 21, 2018

Uh oh!

addaleax commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu Oct 21, 2018

Choose a reason for hiding this comment

Uh oh!

lundibundi Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

mscdex commented Oct 22, 2018

Uh oh!

mscdex commented Oct 22, 2018

Uh oh!

addaleax commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Oct 24, 2018

Uh oh!

addaleax commented Oct 24, 2018

Uh oh!

addaleax commented Oct 24, 2018

Uh oh!

p3x-robot commented Nov 19, 2018

Uh oh!

addaleax commented Nov 19, 2018

Uh oh!

p3x-robot commented Nov 19, 2018

Uh oh!

MylesBorins commented Nov 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

addaleax commented Oct 21, 2018 •

edited

Loading

addaleax commented Oct 21, 2018 •

edited

Loading

lundibundi Oct 22, 2018 •

edited

Loading

addaleax commented Oct 23, 2018 •

edited

Loading

mscdex commented Oct 23, 2018 •

edited

Loading