Skip to content

http: always cork outgoing writes#13522

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
Merged
mscdex merged 1 commit into from
Jun 13, 2017
Merged

http: always cork outgoing writes #13522

mscdex merged 1 commit into from
Jun 13, 2017

Conversation

@mscdex
Copy link
Contributor

@mscdexmscdex 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

@mscdexmscdex 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-botnodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 7, 2017
@mscdexmscdexforce-pushed the http-outgoing-always-cork-writes branch from 110efcf to 1e0f85bCompareJune 7, 2017 16:11
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jun 7, 2017

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

@mscdex
Copy link
ContributorAuthor

mscdex commented Jun 7, 2017

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

@mscdex
Copy link
ContributorAuthor

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
ContributorAuthor

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

@mcollinamcollina 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

@indutnyindutny left a comment

Choose a reason for hiding this comment

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

LGTM

@mscdexmscdexforce-pushed the http-outgoing-always-cork-writes branch from 1e0f85b to be4b583CompareJune 12, 2017 17:55
@mscdex
Copy link
ContributorAuthor

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
@mscdexmscdexforce-pushed the http-outgoing-always-cork-writes branch from be4b583 to c4fc7d9CompareJune 13, 2017 00:42
@mscdexmscdex merged commit c4fc7d9 into nodejs:masterJun 13, 2017
@mscdexmscdex 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
@addaleaxaddaleax 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
@andrewjjenkinsandrewjjenkins mentioned this pull request Aug 2, 2017
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@jasnelljasnelljasnell approved these changes

@cjihrigcjihrigcjihrig approved these changes

@tniessentniessentniessen approved these changes

+1 more reviewer

@indutnyindutnyindutny approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

httpIssues or PRs related to the http subsystem.performanceIssues 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