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

src: improve StreamBase write throughput by addaleax · Pull Request #23843 · nodejs/node · GitHub

/ node Public

src: improve StreamBase write throughput - #23843

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

src: improve StreamBase write throughput#23843
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:net-throughput2

Conversation

addaleax commented Oct 24, 2018

Copy link
Copy Markdown
Member

Improve performance by transferring information about write status
to JS through an AliasedBuffer, rather than object properties
set from C++.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

src: improve StreamBase write throughput
d71f270 
Improve performance by transferring information about write status
to JS through an `AliasedBuffer`, rather than object properties
set from C++.

nodejs-github-bot commented Oct 24, 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/1354/pipeline

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 24, 2018

addaleax commented Oct 24, 2018

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/18099/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/252/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1596/

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

addaleax commented Oct 24, 2018

Copy link
Copy Markdown
Member Author

Benchmarks looked good but had some isolated negative instances – I’d like to make sure these aren’t real performance regressions…

Benchmark CI again (net-c2s.js only, more iterations): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/253/
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18125/

addaleax commented Oct 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Think I figured out why the benchmarks had that weird drop for ASCII writes. (Edit: No, my mistake.)

New benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/254/
(New CI: https://ci.nodejs.org/job/node-test-pull-request/18133/)
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18134/

addaleax commented Oct 26, 2018

Copy link
Copy Markdown
Member Author

So … the most recent full benchmark (https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/254/) shows a significant -8 % regression for one of the large-buffer ASCII variants of a benchmark. Running only that benchmark with a few more iterations (https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/255/) shows a significant 5 % increase. (I did not reproduce either locally.)

I don’t quite know how to make sense of that – I don’t understand how this patch would lead to a regression, and the high significance levels in the different benchmarks would make my first guess that it’s related to how we benchmark things, so I’d feel comfortable going ahead with it (especially in like of the more common significant improvements), but I wanted to point this out in case anybody felt differently.

addaleax commented Oct 27, 2018

Copy link
Copy Markdown
Member Author

Resume CI again: https://ci.nodejs.org/job/node-test-pull-request/18182/

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 27, 2018
targos pushed a commit to targos/node that referenced this pull request Oct 28, 2018
src: improve StreamBase write throughput
f01518e 
Improve performance by transferring information about write status
to JS through an `AliasedBuffer`, rather than object properties
set from C++.

PR-URL: nodejs#23843
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

targos commented Oct 28, 2018

Copy link
Copy Markdown
Member

Landed in f01518e

targos closed this Oct 28, 2018
targos pushed a commit that referenced this pull request Oct 28, 2018
src: improve StreamBase write throughput
5226ba2 
Improve performance by transferring information about write status
to JS through an `AliasedBuffer`, rather than object properties
set from C++.

PR-URL: #23843
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax deleted the net-throughput2 branch October 28, 2018 14:34
targos mentioned this pull request Oct 28, 2018
Merged
targos pushed a commit that referenced this pull request Nov 1, 2018
src: improve StreamBase write throughput
4a79b25 
Improve performance by transferring information about write status
to JS through an `AliasedBuffer`, rather than object properties
set from C++.

PR-URL: #23843
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
This was referenced Nov 2, 2018
Closed
Closed
Closed
Closed
MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v8.x labels Nov 26, 2018

MylesBorins commented Nov 26, 2018

Copy link
Copy Markdown
Contributor

Is this something we would want to backport to LTS? I'm assuming it would likely have to be the entire StreamBase refactor in one go.

MylesBorins added backport-requested-v8.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v10.x labels Nov 26, 2018

addaleax commented Nov 26, 2018

Copy link
Copy Markdown
Member Author

Is this something we would want to backport to LTS?

I think it might be better not to.

I'm assuming it would likely have to be the entire StreamBase refactor in one go.

Not really, no. It’s like all other changes – some commits make sense to backport, others have a nonzero risk of breakage (although I try to run CITGM on all of them).

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
+1 more reviewer
apapirovski apapirovski 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. c++ Issues and PRs that require attention from people who are familiar with C++. 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.

6 participants

Back | FazBrowse Home | New Git URL