FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

src: improve StreamBase read throughput - #23797

Closed
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:net-throughput
ConversationCommits (4)ChecksFiles changed
Closed

src: improve StreamBase read throughput#23797
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:net-throughput

Conversation

addaleax commented Oct 21, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

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 commented Oct 21, 2018

Copy link
Copy Markdown
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 added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 21, 2018

addaleax commented Oct 21, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 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 approved these changes Oct 21, 2018
View reviewed changes

TimothyGu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are there benchmark results available?

Comment thread src/stream_base.cc

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

TimothyGu Oct 21, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lundibundi Oct 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax Oct 23, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mscdex commented Oct 22, 2018

Copy link
Copy Markdown
Contributor

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

mscdex commented Oct 22, 2018

Copy link
Copy Markdown
Contributor

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

addaleax commented Oct 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented Oct 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Oct 24, 2018

Copy link
Copy Markdown
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 commented Oct 24, 2018

Copy link
Copy Markdown
Member Author

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 24, 2018

addaleax commented Oct 24, 2018

Copy link
Copy Markdown
Member Author

Landed in 1365f65

addaleax closed this Oct 24, 2018
addaleax deleted the net-throughput branch October 24, 2018 07:58
addaleax added a commit that referenced this pull request Oct 24, 2018
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
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 mentioned this pull request Oct 27, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
Closed

p3x-robot commented Nov 19, 2018

Copy link
Copy Markdown

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?

p3x-robot mentioned this pull request Nov 19, 2018
Closed

addaleax commented Nov 19, 2018

Copy link
Copy Markdown
Member Author

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

p3x-robot commented Nov 19, 2018

Copy link
Copy Markdown

found it, thanks very much!

MylesBorins commented Nov 26, 2018

Copy link
Copy Markdown
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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+2 more reviewers
lundibundi lundibundi left review comments
TimothyGu TimothyGu approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL