http2: refactor outgoing write mechanism by addaleax · Pull Request #17718 · nodejs/node

addaleax · 2017-12-17T18:51:01Z

The first commits in this are from #17406. It’s blocked on that merge-conflict-wise but generally ready for review.

http2: remove redundant write indirection

nghttp2_stream_write_t was not a necessary redirection layer
and came with the cost of one additional allocation per stream write.

Also, having both nghttp2_stream_write and nghttp2_stream_write_t
as identifiers did not help with readability.

  $ ./node benchmark/compare.js --new ./node --old ./node-before --runs 100 --filter write --set streams=20 --set length=1024 --set size=16 http2 | Rscript benchmark/compare.R
  [00:26:09|% 100| 1/1 files | 200/200 runs | 1/1 configs]: Done
                                                                       improvement confidence     p.value
   http2/write.js benchmarker="h2load" size=16 length=1024 streams=20      0.58 %         ** 0.005535241

http2: refactor outgoing write mechanism

Only finish outgoing WriteWraps once data has actually been
passed to the underlying socket.
- This makes HTTP2 streams respect backpressure
Use DoTryWrite as a shortcut for sending out as much of
the data synchronously without blocking as possible

Use NGHTTP2_DATA_FLAG_NO_COPY to avoid copying DATA frame
contents into nghttp2’s buffers before sending them out.

$ ./node benchmark/compare.js --new ./node --old ./node-before --runs 10 --filter write --set streams=20 --set length=1024 --set size=16 http2 | Rscript benchmark/compare.R
[00:02:30|% 100| 1/1 files | 20/20 runs | 1/1 configs]: Done
                                                                     improvement confidence      p.value
 http2/write.js benchmarker="h2load" size=16 length=1024 streams=20      6.88 %        *** 2.261566e-08

Checklist

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

Affected core subsystem(s)

http2 @nodejs/http2

CI: https://ci.nodejs.org/job/node-test-commit/14902/

jasnell · 2017-12-18T16:47:57Z

src/node_http2.cc

nit... line up the args :-)

@jasnell done, and rebased :)

jasnell · 2017-12-18T17:23:06Z

I love it. Initial read through looks good but will give a more thorough review a bit later on today.

jasnell · 2017-12-18T18:26:55Z

#17406 has landed. This can be rebased to remove those carried over commits :-)

`nghttp2_stream_write_t` was not a necessary redirection layer and came with the cost of one additional allocation per stream write. Also, having both `nghttp2_stream_write` and `nghttp2_stream_write_t` as identifiers did not help with readability. $ ./node benchmark/compare.js --new ./node --old ./node-before --runs 100 --filter write --set streams=20 --set length=1024 --set size=16 http2 | Rscript benchmark/compare.R [00:26:09|% 100| 1/1 files | 200/200 runs | 1/1 configs]: Done improvement confidence p.value http2/write.js benchmarker="h2load" size=16 length=1024 streams=20 0.58 % ** 0.005535241

- Only finish outgoing `WriteWrap`s once data has actually been passed to the underlying socket. - This makes HTTP2 streams respect backpressure - Use `DoTryWrite` as a shortcut for sending out as much of the data synchronously without blocking as possible - Use `NGHTTP2_DATA_FLAG_NO_COPY` to avoid copying DATA frame contents into nghttp2’s buffers before sending them out. $ ./node benchmark/compare.js --new ./node --old ./node-before --runs 10 --filter write --set streams=20 --set length=1024 --set size=16 http2 | Rscript benchmark/compare.R [00:02:30|% 100| 1/1 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value http2/write.js benchmarker="h2load" size=16 length=1024 streams=20 6.88 % *** 2.261566e-08

addaleax · 2017-12-18T22:44:58Z

CI: https://ci.nodejs.org/job/node-test-commit/14930/

jasnell · 2017-12-19T00:03:02Z

src/node_http2.cc

  // fails.
  CHECK_EQ(fn(&session_, callbacks, this, *opts), 0);
+
+  outgoing_storage_.reserve(4096);


may want to play with some different numbers here. The default max frame size is 16k.

@jasnell This storage is only for data that comes out of nghttp2, not the actual DATA frame payload … and I’m just realizing that padding could also just come from a static buffer. :)

jasnell

Looks good. thanks for this. It was the next big chunk to work on. :-)

jasnell · 2017-12-19T23:58:04Z

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

jasnell · 2017-12-20T00:49:11Z

CI looks good except for some build bot failures on linux, trying again on those just to be safe: https://ci.nodejs.org/job/node-test-commit-linux/15097/

`nghttp2_stream_write_t` was not a necessary redirection layer and came with the cost of one additional allocation per stream write. Also, having both `nghttp2_stream_write` and `nghttp2_stream_write_t` as identifiers did not help with readability. PR-URL: #17718 Reviewed-By: James M Snell <jasnell@gmail.com>

- Only finish outgoing `WriteWrap`s once data has actually been passed to the underlying socket. - This makes HTTP2 streams respect backpressure - Use `DoTryWrite` as a shortcut for sending out as much of the data synchronously without blocking as possible - Use `NGHTTP2_DATA_FLAG_NO_COPY` to avoid copying DATA frame contents into nghttp2’s buffers before sending them out. PR-URL: #17718 Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2017-12-20T01:39:19Z

Landed in e3b44b6 and e0e6b68

`nghttp2_stream_write_t` was not a necessary redirection layer and came with the cost of one additional allocation per stream write. Also, having both `nghttp2_stream_write` and `nghttp2_stream_write_t` as identifiers did not help with readability. PR-URL: nodejs#17718 Reviewed-By: James M Snell <jasnell@gmail.com>

- Only finish outgoing `WriteWrap`s once data has actually been passed to the underlying socket. - This makes HTTP2 streams respect backpressure - Use `DoTryWrite` as a shortcut for sending out as much of the data synchronously without blocking as possible - Use `NGHTTP2_DATA_FLAG_NO_COPY` to avoid copying DATA frame contents into nghttp2’s buffers before sending them out. PR-URL: nodejs#17718 Reviewed-By: James M Snell <jasnell@gmail.com>

`nghttp2_stream_write_t` was not a necessary redirection layer and came with the cost of one additional allocation per stream write. Also, having both `nghttp2_stream_write` and `nghttp2_stream_write_t` as identifiers did not help with readability. Backport-PR-URL: #18050 PR-URL: #17718 Reviewed-By: James M Snell <jasnell@gmail.com>

- Only finish outgoing `WriteWrap`s once data has actually been passed to the underlying socket. - This makes HTTP2 streams respect backpressure - Use `DoTryWrite` as a shortcut for sending out as much of the data synchronously without blocking as possible - Use `NGHTTP2_DATA_FLAG_NO_COPY` to avoid copying DATA frame contents into nghttp2’s buffers before sending them out. Backport-PR-URL: #18050 PR-URL: #17718 Reviewed-By: James M Snell <jasnell@gmail.com>

`nghttp2_stream_write_t` was not a necessary redirection layer and came with the cost of one additional allocation per stream write. Also, having both `nghttp2_stream_write` and `nghttp2_stream_write_t` as identifiers did not help with readability. Backport-PR-URL: #18050 PR-URL: #17718 Reviewed-By: James M Snell <jasnell@gmail.com>

- Only finish outgoing `WriteWrap`s once data has actually been passed to the underlying socket. - This makes HTTP2 streams respect backpressure - Use `DoTryWrite` as a shortcut for sending out as much of the data synchronously without blocking as possible - Use `NGHTTP2_DATA_FLAG_NO_COPY` to avoid copying DATA frame contents into nghttp2’s buffers before sending them out. Backport-PR-URL: #18050 PR-URL: #17718 Reviewed-By: James M Snell <jasnell@gmail.com>

`nghttp2_stream_write_t` was not a necessary redirection layer and came with the cost of one additional allocation per stream write. Also, having both `nghttp2_stream_write` and `nghttp2_stream_write_t` as identifiers did not help with readability. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17718 Reviewed-By: James M Snell <jasnell@gmail.com>

- Only finish outgoing `WriteWrap`s once data has actually been passed to the underlying socket. - This makes HTTP2 streams respect backpressure - Use `DoTryWrite` as a shortcut for sending out as much of the data synchronously without blocking as possible - Use `NGHTTP2_DATA_FLAG_NO_COPY` to avoid copying DATA frame contents into nghttp2’s buffers before sending them out. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17718 Reviewed-By: James M Snell <jasnell@gmail.com>

`nghttp2_stream_write_t` was not a necessary redirection layer and came with the cost of one additional allocation per stream write. Also, having both `nghttp2_stream_write` and `nghttp2_stream_write_t` as identifiers did not help with readability. Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17718 Reviewed-By: James M Snell <jasnell@gmail.com>

- Only finish outgoing `WriteWrap`s once data has actually been passed to the underlying socket. - This makes HTTP2 streams respect backpressure - Use `DoTryWrite` as a shortcut for sending out as much of the data synchronously without blocking as possible - Use `NGHTTP2_DATA_FLAG_NO_COPY` to avoid copying DATA frame contents into nghttp2’s buffers before sending them out. Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17718 Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added blocked PRs that are blocked by other issues or PRs. http2 Issues or PRs related to the http2 subsystem. performance Issues and PRs related to the performance of Node.js. labels Dec 17, 2017

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 17, 2017

addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 17, 2017

addaleax mentioned this pull request Dec 18, 2017

http2: refactor close/destroy for Http2Stream and Http2Session #17406

Closed

4 tasks

jasnell reviewed Dec 18, 2017

View reviewed changes

addaleax added 2 commits December 18, 2017 23:27

addaleax force-pushed the http2-send-refactor branch from beca4ab to 548cacf Compare December 18, 2017 22:38

addaleax removed the blocked PRs that are blocked by other issues or PRs. label Dec 18, 2017

jasnell reviewed Dec 19, 2017

View reviewed changes

jasnell approved these changes Dec 19, 2017

View reviewed changes

[squash] use static zero-initialized buffer for padding

906b6b3

jasnell closed this Dec 20, 2017

addaleax deleted the http2-send-refactor branch December 23, 2017 16:59

addaleax mentioned this pull request Dec 23, 2017

Investigate test-http2-createwritereq failure on Linux and AIX #17840

Closed

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added the dont-land-on-v6.x label Jan 19, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

http2: refactor outgoing write mechanism#17718

http2: refactor outgoing write mechanism#17718
addaleax wants to merge 3 commits intonodejs:masterfrom
addaleax:http2-send-refactor

addaleax commented Dec 17, 2017 •

edited

Loading

Uh oh!

jasnell Dec 18, 2017

Uh oh!

addaleax Dec 18, 2017

Uh oh!

jasnell commented Dec 18, 2017

Uh oh!

jasnell commented Dec 18, 2017

Uh oh!

addaleax commented Dec 18, 2017

Uh oh!

jasnell Dec 19, 2017

Uh oh!

addaleax Dec 19, 2017

Uh oh!

jasnell left a comment

Uh oh!

jasnell commented Dec 19, 2017

Uh oh!

jasnell commented Dec 20, 2017

Uh oh!

jasnell commented Dec 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

addaleax commented Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

jasnell Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 18, 2017

Uh oh!

jasnell commented Dec 18, 2017

Uh oh!

addaleax commented Dec 18, 2017

Uh oh!

jasnell Dec 19, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 19, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 19, 2017

Uh oh!

jasnell commented Dec 20, 2017

Uh oh!

jasnell commented Dec 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

addaleax commented Dec 17, 2017 •

edited

Loading