http2: code style and performance improvements #16239

jasnell · 2017-10-16T19:22:36Z

A number of code style and performance improvements to the http2 internals.

A significant chunk of this involves removing some of the safety checks that were left in while things were being developed. Because the code has stablized, these are less necessary. Removing these yields a significant performance improvement.

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)

http2

mcollina

LGTM. how much do we gain by disabling the checks?

apapirovski

👍

apapirovski · 2017-10-16T21:00:50Z

benchmark/http2/headers.js

@@ -6,7 +6,8 @@ const PORT = common.PORT;
 const bench = common.createBenchmark(main, {
  n: [1e3],
  nheaders: [0, 10, 100, 1000],
-}, { flags: ['--no-warnings'] });
+  benchmarker: ['h2load']
+}, { flags: ['--no-warnings', '--expose-http2'] });


Any specific reason --expose-http2 is back in? Is it just so compare can be run against versions where that was required?

Yep. It's for benchmark comparisons against v8.x

jasnell · 2017-10-16T21:03:50Z

how much do we gain by disabling the checks?

About an extra ~1k reqs per second.

With the checks and if-branches removed, I'm seeing a boost of about ~7k reqs per second with my limited benchmarking.

jasnell · 2017-10-16T21:04:26Z

Almost all of these checks are in hot paths btw

jasnell · 2017-10-17T21:17:57Z

@mcollina ... please take another look at this, in particular the last commit. All current tests are passing and performance is improved... we'll need to test this further tho.

jasnell · 2017-10-18T22:55:03Z

any reason for using std::list here?

No specific reason, and no specific preference, to be honest :-)

jasnell · 2017-10-18T23:14:16Z

@addaleax ... there... now using deque ;-)

addaleax · 2017-10-19T20:52:49Z

src/node_http2_core.h

-    CHECK_EQ(data_chunks_head_, nullptr);
-    CHECK_EQ(data_chunks_tail_, nullptr);
-    CHECK_EQ(current_headers_head_, nullptr);
-    CHECK_EQ(current_headers_tail_, nullptr);


These are supposed to check emptyness, right? That can still happen if we like.

Yeah, but they're largely worthless checks. The Nghttp2Stream instances are pooled are only rarely actually destroyed so these checks aren't really helping us with much. I had them in there largely to test for correctness as I was writing the code.

addaleax

jasnell · 2017-10-20T21:38:32Z

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

jasnell · 2017-10-20T22:32:43Z

The misbehaving flow control tests are failing on Windows so I'll need to get that checked out before this can land.

jasnell · 2017-10-22T17:55:13Z

~~New CI: https://ci.nodejs.org/job/node-test-pull-request/10907/~~
Once more, with feeling: https://ci.nodejs.org/job/node-test-pull-request/10908/

Several if checks were left in while the code was being developed. Now that the core API has stablized more, the checks are largely unnecessary and can be removed, yielding a significant boost in performance.

apapirovski

Changes look good, confirming my approval.

PR-URL: #16239 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

* move CHECK statements into DEBUG checks * improve performance by removing branches * Several if checks were left in while the code was being developed. Now that the core API has stablized more, the checks are largely unnecessary and can be removed, yielding a significant boost in performance. * refactor flow control for proper backpressure * use std::queue for inbound headers * use std::queue for outbound data * remove now unnecessary FreeHeaders function * expand comments and miscellaneous edits * add a couple of misbehaving flow control tests PR-URL: #16239 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

jasnell · 2017-10-23T19:28:09Z

Landed in 82b1660 and f16b9c1

PR-URL: #16239 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

* move CHECK statements into DEBUG checks * improve performance by removing branches * Several if checks were left in while the code was being developed. Now that the core API has stablized more, the checks are largely unnecessary and can be removed, yielding a significant boost in performance. * refactor flow control for proper backpressure * use std::queue for inbound headers * use std::queue for outbound data * remove now unnecessary FreeHeaders function * expand comments and miscellaneous edits * add a couple of misbehaving flow control tests PR-URL: #16239 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs/node#16239 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

* move CHECK statements into DEBUG checks * improve performance by removing branches * Several if checks were left in while the code was being developed. Now that the core API has stablized more, the checks are largely unnecessary and can be removed, yielding a significant boost in performance. * refactor flow control for proper backpressure * use std::queue for inbound headers * use std::queue for outbound data * remove now unnecessary FreeHeaders function * expand comments and miscellaneous edits * add a couple of misbehaving flow control tests PR-URL: nodejs/node#16239 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs/node#16239 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

* move CHECK statements into DEBUG checks * improve performance by removing branches * Several if checks were left in while the code was being developed. Now that the core API has stablized more, the checks are largely unnecessary and can be removed, yielding a significant boost in performance. * refactor flow control for proper backpressure * use std::queue for inbound headers * use std::queue for outbound data * remove now unnecessary FreeHeaders function * expand comments and miscellaneous edits * add a couple of misbehaving flow control tests PR-URL: nodejs/node#16239 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 16, 2017

jasnell requested a review from mcollina October 16, 2017 19:22

mcollina approved these changes Oct 16, 2017

View reviewed changes

apapirovski approved these changes Oct 16, 2017

View reviewed changes

jasnell force-pushed the http2-improvements2 branch from dcf716a to 5652326 Compare October 18, 2017 20:02

addaleax reviewed Oct 19, 2017

View reviewed changes

addaleax approved these changes Oct 19, 2017

View reviewed changes

jasnell added 12 commits October 22, 2017 11:07

benchmark: improve http2 benchmark configs

d8120c3

http2: move CHECK statements into DEBUG checks

791d4b4

http2: code style fix

04179bf

http2: improve code documentation slightly

18b194f

http2: improve performance by removing branches

1bf393e

Several if checks were left in while the code was being developed. Now that the core API has stablized more, the checks are largely unnecessary and can be removed, yielding a significant boost in performance.

http2: refactor flow control for proper backpressure

a3f5864

http2: use std::queue for inbound headers

04f2b2a

http2: use std::queue for outbound data

e150087

http2: remove now unnecessary FreeHeaders function

c19d914

http2: expand comments and miscellaneous edits

796e313

http2: use std::queue instead of std::list

948606f

[Squash] Fix tests on Windows

9086258

jasnell force-pushed the http2-improvements2 branch from 8102281 to 9086258 Compare October 22, 2017 18:10

apapirovski approved these changes Oct 23, 2017

View reviewed changes

jasnell added a commit that referenced this pull request Oct 23, 2017

benchmark: improve http2 benchmark configs

82b1660

PR-URL: #16239 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

jasnell closed this Oct 23, 2017

MylesBorins pushed a commit that referenced this pull request Oct 23, 2017

benchmark: improve http2 benchmark configs

5d34f2f

PR-URL: #16239 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: code style and performance improvements #16239

http2: code style and performance improvements #16239

jasnell commented Oct 16, 2017

mcollina left a comment

apapirovski left a comment

apapirovski Oct 16, 2017

jasnell Oct 16, 2017

jasnell commented Oct 16, 2017

jasnell commented Oct 16, 2017

jasnell commented Oct 17, 2017

jasnell commented Oct 18, 2017

jasnell commented Oct 18, 2017

addaleax Oct 19, 2017

jasnell Oct 20, 2017

addaleax left a comment

jasnell commented Oct 20, 2017

jasnell commented Oct 20, 2017

jasnell commented Oct 22, 2017 •

edited

Loading

apapirovski left a comment

jasnell commented Oct 23, 2017

http2: code style and performance improvements #16239

http2: code style and performance improvements #16239

Conversation

jasnell commented Oct 16, 2017

Checklist

Affected core subsystem(s)

mcollina left a comment

Choose a reason for hiding this comment

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Oct 16, 2017

Choose a reason for hiding this comment

jasnell Oct 16, 2017

Choose a reason for hiding this comment

jasnell commented Oct 16, 2017

jasnell commented Oct 16, 2017

jasnell commented Oct 17, 2017

jasnell commented Oct 18, 2017

jasnell commented Oct 18, 2017

addaleax Oct 19, 2017

Choose a reason for hiding this comment

jasnell Oct 20, 2017

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

jasnell commented Oct 20, 2017

jasnell commented Oct 20, 2017

jasnell commented Oct 22, 2017 • edited Loading

apapirovski left a comment

Choose a reason for hiding this comment

jasnell commented Oct 23, 2017

jasnell commented Oct 22, 2017 •

edited

Loading