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

http: always cork outgoing writes #13522

Merged
merged 1 commit into from
Jun 13, 2017

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Jun 7, 2017

This PR always corks outgoing writes, no matter whether it's using chunked encoding or has a Content-Length set. Previously it was only enabled for chunked encoding writes.

Some benchmark results:

                                                                                                          improvement confidence      p.value
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=1 len=1024 type="buffer" benchmarker="wrk"       -0.86 %            1.738663e-01
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=1 len=102400 type="buffer" benchmarker="wrk"     -1.38 %          * 1.096846e-02
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=1 len=4 type="buffer" benchmarker="wrk"           4.46 %        *** 1.601082e-05
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=4 len=1024 type="buffer" benchmarker="wrk"     1662.88 %        *** 2.753497e-48
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=4 len=102400 type="buffer" benchmarker="wrk"    987.47 %        *** 1.450546e-35
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=4 len=4 type="buffer" benchmarker="wrk"        1691.00 %        *** 1.523719e-50

The single chunk write results here that are < 0% will be even less of a problem once the nextTick performance improvements in #13446 land:

                                                                                                          improvement confidence      p.value
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=1 len=1024 type="buffer" benchmarker="wrk"        0.62 %            2.106693e-01
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=1 len=102400 type="buffer" benchmarker="wrk"     -1.11 %            5.027460e-02
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=1 len=4 type="buffer" benchmarker="wrk"           5.28 %        *** 1.522987e-05
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=4 len=1024 type="buffer" benchmarker="wrk"     1679.19 %        *** 1.692272e-53
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=4 len=102400 type="buffer" benchmarker="wrk"   1010.63 %        *** 1.096472e-38
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=4 len=4 type="buffer" benchmarker="wrk"        1703.22 %        *** 1.768577e-51

CI: https://ci.nodejs.org/job/node-test-pull-request/8532/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • http

@mscdex mscdex added http Issues or PRs related to the http subsystem. performance Issues and PRs related to the performance of Node.js. labels Jun 7, 2017
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 7, 2017
@mscdex mscdex force-pushed the http-outgoing-always-cork-writes branch from 110efcf to 1e0f85b Compare June 7, 2017 16:11
@Fishrock123
Copy link
Contributor

This might increase overall throughput, but does it possibly increase latency?

@mscdex
Copy link
Contributor Author

mscdex commented Jun 7, 2017

@Fishrock123 No more than it already was for a chunked encoding write.

@mscdex
Copy link
Contributor Author

mscdex commented Jun 8, 2017

Just checked this PR against V8 5.9 that is now in master and I get these results without the changes from the previously linked nextTick() PR:

                                                                                                          improvement confidence      p.value
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=1 len=1024 type="buffer" benchmarker="wrk"       -0.03 %            9.662854e-01
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=1 len=102400 type="buffer" benchmarker="wrk"      1.22 %            5.531567e-01
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=1 len=4 type="buffer" benchmarker="wrk"           7.83 %        *** 3.200630e-06
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=4 len=1024 type="buffer" benchmarker="wrk"     1680.93 %        *** 2.414472e-43
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=4 len=102400 type="buffer" benchmarker="wrk"    988.62 %        *** 6.840740e-36
 http/simple.js res="normal" chunkedEnc="false" c=50 chunks=4 len=4 type="buffer" benchmarker="wrk"        1728.47 %        *** 2.094178e-51

@mscdex
Copy link
Contributor Author

mscdex commented Jun 9, 2017

/cc @nodejs/collaborators

@mcollina
Copy link
Member

mcollina commented Jun 9, 2017

Prior art: #7946.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM as semver-minor.

I think it might require extensive perf testing before backporting to v6.

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@mscdex
Copy link
Contributor Author

mscdex commented Jun 12, 2017

PR-URL: nodejs#13522
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@mscdex mscdex force-pushed the http-outgoing-always-cork-writes branch from be4b583 to c4fc7d9 Compare June 13, 2017 00:42
@mscdex mscdex merged commit c4fc7d9 into nodejs:master Jun 13, 2017
@mscdex mscdex deleted the http-outgoing-always-cork-writes branch June 13, 2017 00:45
addaleax pushed a commit that referenced this pull request Jun 17, 2017
PR-URL: #13522
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@addaleax addaleax mentioned this pull request Jun 17, 2017
addaleax pushed a commit that referenced this pull request Jun 21, 2017
PR-URL: #13522
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 24, 2017
PR-URL: #13522
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
rvagg pushed a commit that referenced this pull request Jun 29, 2017
PR-URL: #13522
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #13522
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http 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.

8 participants