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.

Sign up for GitHub

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

Jump to bottom

src: improve StreamBase read throughput #23797

Closed
wants to merge 4 commits into from
Closed

src: improve StreamBase read throughput #23797

wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 21, 2018 •
edited
Loading

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

@addaleax
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
Copy link
Collaborator

@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

@nodejs-github-bot 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
Copy link
Member Author

addaleax commented Oct 21, 2018 •
edited
Loading

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/

@addaleax 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
TimothyGu
TimothyGu approved these changes Oct 21, 2018
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there benchmark results available?

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@lundibundi lundibundi Oct 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mscdex
Copy link
Contributor

mscdex commented Oct 22, 2018

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

@mscdex
Copy link
Contributor

mscdex commented Oct 22, 2018

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

@addaleax
Copy link
Member Author

addaleax commented Oct 23, 2018 •
edited
Loading

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.)

@mscdex
Copy link
Contributor

mscdex commented Oct 23, 2018 •
edited
Loading

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
Copy link
Member Author

@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
Copy link
Member Author

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

@addaleax 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
Copy link
Member Author

Landed in 1365f65

@addaleax addaleax closed this Oct 24, 2018
@addaleax addaleax deleted the net-throughput branch October 24, 2018 07:58
addaleax added a commit that referenced this pull request Oct 24, 2018
@addaleax
src: improve StreamBase read throughput
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>
targos pushed a commit that referenced this pull request Oct 24, 2018
@addaleax @targos
src: improve StreamBase read throughput
48ed81f 
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>
@targos targos mentioned this pull request Oct 27, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
@beevelop beevelop mentioned this pull request Nov 2, 2018
Closed
@p3x-robot
Copy link

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

@p3x-robot p3x-robot mentioned this pull request Nov 19, 2018
Closed
@addaleax
Copy link
Member Author

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

@p3x-robot
Copy link

found it, thanks very much!

@MylesBorins
Copy link
Contributor

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.

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

@lundibundi lundibundi lundibundi left review comments

@TimothyGu TimothyGu TimothyGu approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. net Issues and PRs related to the net subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@addaleax @nodejs-github-bot @mscdex @p3x-robot @MylesBorins @jasnell @TimothyGu @lundibundi