GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: improve StreamBase read throughput by addaleax · Pull Request #23797 · nodejs/node

Conversation

@addaleax

@addaleax addaleax commented Oct 21, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca, bricss, paambaati, and mmarchini reacted with heart emoji
All reactions
  • ❤️ 4 reactions
@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

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Oct 21, 2018
@addaleax

addaleax commented Oct 21, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@addaleax
fixup! src: improve StreamBase read throughput
6023661
TimothyGu
TimothyGu approved these changes Oct 21, 2018
View reviewed changes

@TimothyGu TimothyGu left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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 TimothyGu Oct 21, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lundibundi lundibundi Oct 22, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Oct 23, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lundibundi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
jasnell
jasnell approved these changes Oct 21, 2018
View reviewed changes
@mscdex

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Oct 22, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax
fixup! src: improve StreamBase read throughput
18069e4
@addaleax
addaleax force-pushed the net-throughput branch from 2350bd1 to 18069e4 Compare October 23, 2018 03:17
@addaleax

addaleax commented Oct 23, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax
fixup! src: improve StreamBase read throughput
f829b61
@mscdex

mscdex commented Oct 23, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Oct 24, 2018

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 24, 2018
@addaleax

addaleax commented Oct 24, 2018

Copy link
Copy Markdown
Member Author

Landed in 1365f65
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Oct 27, 2018
Release proposal: v11.1.0 #23922
Merged
This was referenced Nov 2, 2018
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#245
Closed
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#246
Closed
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#247
Closed
2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#248
Closed
@p3x-robot

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@p3x-robot p3x-robot mentioned this pull request Nov 19, 2018
SPDY fails in node >= 11.1.0 spdy-http2/node-spdy#350
Closed
@addaleax

addaleax commented Nov 19, 2018

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@p3x-robot

p3x-robot commented Nov 19, 2018

Copy link
Copy Markdown

found it, thanks very much!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins added lts-watch-v8.x labels Nov 26, 2018
@MylesBorins

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins added backport-requested-v8.x and removed lts-watch-v10.x labels Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@addaleax @nodejs-github-bot @mscdex @p3x-robot @MylesBorins @jasnell @TimothyGu @lundibundi
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.