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.

Sign up for GitHub

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

Jump to bottom

http: always cork outgoing writes #13522

Merged
merged 1 commit into from
Jun 13, 2017
Merged

http: always cork outgoing writes #13522

merged 1 commit into from
Jun 13, 2017

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Jun 7, 2017 •
edited
Loading

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 •
edited
Loading

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.

mcollina
mcollina approved these changes Jun 9, 2017
View reviewed changes
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.

indutny
indutny approved these changes Jun 9, 2017
View reviewed changes
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 mscdex force-pushed the http-outgoing-always-cork-writes branch from 1e0f85b to be4b583 Compare June 12, 2017 17:55
@mscdex
Copy link
Contributor Author

mscdex commented Jun 12, 2017

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/8613/

@mscdex
http: always cork outgoing writes
c4fc7d9 
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
@mscdex @addaleax
http: always cork outgoing writes
6aa790c 
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
Merged
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@mscdex @addaleax
http: always cork outgoing writes
9d002c2 
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
@mscdex @addaleax
http: always cork outgoing writes
93c1774 
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
@mscdex @rvagg
http: always cork outgoing writes
02a62da 
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
@mscdex @addaleax
http: always cork outgoing writes
b22a04b 
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>
@andrewjjenkins andrewjjenkins mentioned this pull request Aug 2, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@indutny indutny indutny approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http 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
@mscdex @Fishrock123 @mcollina @indutny @jasnell @cjihrig @tniessen @nodejs-github-bot