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.

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

src: improve StreamBase write throughput #23843

Closed
wants to merge 2 commits into from

Conversation

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

Improve performance by transferring information about write status
to JS through an `AliasedBuffer`, rather than object properties
set from C++.
@nodejs-github-bot 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 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
Copy link
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
Copy link
Member Author

addaleax commented Oct 25, 2018

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 27, 2018
targos pushed a commit to targos/node that referenced this pull request Oct 28, 2018
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
Copy link
Member

targos commented Oct 28, 2018

Landed in f01518e

@targos targos closed this Oct 28, 2018
targos pushed a commit that referenced this pull request Oct 28, 2018
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 addaleax deleted the net-throughput2 branch October 28, 2018 14:34
targos pushed a commit that referenced this pull request Nov 1, 2018
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>
@MylesBorins 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
Copy link
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 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
Copy link
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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
Development

Successfully merging this pull request may close these issues.

6 participants