Reuse detected keepalive of http_parser as long as possible #299

normanmaurer · 2018-04-10T17:25:24Z

Motivation:

To detect if keepalive is used we need to search the headers (if HTTP/1.1 is used). This may be expensive depending how many headers are present. http_parser itself detects already if keep alive should be used and so we can re-use this as long as the user did not modify the headers.

Modifications:

Reuse keepalive parsed by http_parser as long as the headers were not modified.

Result:

Less overhead as long as the headers are not modified.

normanmaurer · 2018-04-10T17:26:18Z

Also @tanner0101

This gives some perf win in my benchmarks. Let me know what you think about this idea @tanner0101 @weissi @Lukasa

BasThomas · 2018-04-10T18:22:04Z

Sources/NIOHTTP1/HTTPTypes.swift

+fileprivate enum HTTPHeadersModificationState {
+    case modifiedNonContinuous
+    case modified
+    case notmodified


Why did you choose for notmodified over notModified?

no idea.. let me fix :)

BasThomas · 2018-04-10T18:22:25Z

Sources/NIOHTTP1/HTTPTypes.swift

+        switch newState {
+        case .modified where self.state == .notmodified:
+            self.state = newState
+        case .modifiedNonContinuous:


Any reason to not group this with the above case?

good idea :)

Lukasa

So I generally think this is a good idea. I did wonder whether we should also check headers being added/removed to see if they modify the state, but ultimately I don't think it's worthwhile.

Lukasa · 2018-04-11T11:51:15Z

Sources/NIOHTTP1/HTTPTypes.swift

+            fatalError("Should never be called with \(newState)")
+        default:
+            break
+        }


I tend to think this kind of method is nicer on the enum. Also no need to say "ifNeeded" in the name.

In fact, it may be nicer just to have functions that explain what they are, e.g. state.appendedHeader() and state.removedHeader(). Then you can have some computed vars that use the right names, e.g state.continuous and state.wasModified.

normanmaurer · 2018-04-11T12:22:47Z

@Lukasa addressed... good idea

BasThomas · 2018-04-11T12:31:53Z

Sources/NIOHTTP1/HTTPTypes.swift

+
+    mutating func addedHeader() {
+        switch self {
+        case .notModified:


What about explicitly stating an if-else?

if case .notModified = self { self = .modified }

without the (then unnecessary) return?

Lukasa

Some minor nits.

Lukasa · 2018-04-11T13:16:51Z

Sources/NIOHTTP1/HTTPTypes.swift

@@ -38,13 +38,17 @@ public struct HTTPRequestHead: Equatable {
    /// The header fields for this HTTP request.
    public var headers: HTTPHeaders

+    /// The keepAlive that may be parsed by http_parser before. This is only safe to be used as long as
+    /// the user did not modify the headers yet.


Let's update this comment slightly:

/// The keepAlive value that was detected by http_parser. This can only be used safely /// as long as the user has not made modifications to this header block.

Lukasa · 2018-04-11T13:17:08Z

Sources/NIOHTTP1/HTTPTypes.swift

@@ -202,6 +211,31 @@ private extension UInt8 {
    }
 }

+fileprivate enum HTTPHeadersModificationState {


Mind adding some quick doc comments here?

Lukasa · 2018-04-11T13:17:28Z

Sources/NIOHTTP1/HTTPTypes.swift

+    }
+
+    mutating func removedHeader() {
+        if self != .modifiedNonContinuous {


Why doesn't this use if case?

normanmaurer · 2018-04-11T13:26:52Z

@Lukasa addressed

weissi

I'd prefer to have the state tracking in one place and not to store a potentially wrong keepAlive value

weissi · 2018-04-11T13:34:46Z

Sources/NIOHTTP1/HTTPTypes.swift

+    case modifiedNonContinuous
+    /// Was modified and the underlying storage is continous.
+    case modified
+


nit: why a space here?

weissi · 2018-04-11T14:04:09Z

Sources/NIOHTTP1/HTTPTypes.swift

@@ -38,13 +38,17 @@ public struct HTTPRequestHead: Equatable {
    /// The header fields for this HTTP request.
    public var headers: HTTPHeaders

+    /// The keepAlive value that was detected by http_parser. This can only be used safely
+    /// as long as the user has not made modifications to this header block.
+    private let keepAlive: Bool?


I don't really like having a state variable here that is only valid when also consulting another variable. I'd say the whole state should live in the headers. Couldn't we architect it this way: We allow (as an internal constructor) to pre-seed the headers with a keep alive state. Whenever the headers change, we invalidate that.

So the keep-alive state in the headers could be one of the following:

keepAlive: no header scan needed, I know I have a Connection: keep-alive header

close: no header scan needed, I know I have a Connection: close header

default: no header scan needed, I know I had neither Connection: keep-alive nor Connection: close

needsScan: header scan needed

so internally we could pre-seed it with either keepAlive, close, or default, depending on what http_parser tells us. Then on any modification we change it to needsScan.

@weissi sounds good... will do later today :)

normanmaurer · 2018-04-11T18:20:14Z

@Lukasa @weissi PTAL again

Lukasa · 2018-04-12T11:49:57Z

Sources/NIOHTTP1/HTTPTypes.swift

+    // We know we should close the connection.
+    case close
+    // We need to scan the headers to find out if keep alive is used or not
+    case needScan(continuous: Bool)


Why is there a continuous flag here? You have a HTTPHeadersModificationState that tracks this.

Because I forgot to remove the modification state... it’s all tracked via this one now

I don't like this much. I'd rather this bool was moved out of this enum: it just has nothing to do with what this enum is tracking.

If we do that, we can then name the third case of this enum unknown, and then the implementation of HTTPHeaders.isKeepAlive gets a lot nicer because it can just return .unknown when it doesn't know the answer instead of being passed the HTTP version.

Lukasa · 2018-04-12T13:07:09Z

@swift-nio-bot test this please

Lukasa · 2018-04-12T13:10:53Z

Sources/NIOHTTP1/HTTPTypes.swift

+    // We know we should close the connection.
+    case close
+    // We need to scan the headers to find out if keep alive is used or not
+    case needScan(continuous: Bool)


I don't like this much. I'd rather this bool was moved out of this enum: it just has nothing to do with what this enum is tracking.

If we do that, we can then name the third case of this enum unknown, and then the implementation of HTTPHeaders.isKeepAlive gets a lot nicer because it can just return .unknown when it doesn't know the answer instead of being passed the HTTP version.

normanmaurer · 2018-04-12T17:59:21Z

@Lukasa @weissi need to investigate as it actually makes things slower now ?!?!?

weissi · 2018-04-27T11:33:48Z

@normanmaurer ping, what’s up with this friend?

normanmaurer · 2018-04-27T11:42:43Z

@weissi I need to investigate as it actually resulted in a slowdown after rebasing

weissi · 2018-04-27T12:59:58Z

@normanmaurer you probably want to use the isolated (no network) perf test to verify that. Otherwise there’ll be too much noise I suspect

tomerd · 2018-04-30T20:36:10Z

@swift-nio-bot test this please

normanmaurer · 2018-05-14T13:18:21Z

Alright updated please take a look again @weissi @Lukasa

Before:

 wrk -c 256 -d 10s -t 4 -s ~/Downloads/pipeline-many.lua -H "X-Host: SomeValue" -H "Host: swiftnio.io" -H "ThereAreEvenMoreHeaders: AndMoreValues" http://127.0.0.1:8888
Running 10s test @ http://127.0.0.1:8888
  4 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   104.83ms  209.95ms   2.00s    90.27%
    Req/Sec   132.15k    31.57k  346.95k    88.41%
  5234833 requests in 10.04s, 249.62MB read
  Socket errors: connect 0, read 162, write 0, timeout 17
Requests/sec: 521428.30
Transfer/sec:     24.86MB

with this pr:

wrk -c 256 -d 10s -t 4 -s ~/Downloads/pipeline-many.lua -H "X-Host: SomeValue" -H "Host: swiftnio.io" -H "ThereAreEvenMoreHeaders: AndMoreValues" http://127.0.0.1:8888
Running 10s test @ http://127.0.0.1:8888
  4 threads and 256 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   105.65ms  215.06ms   1.99s    91.27%
    Req/Sec   137.61k    32.64k  329.69k    89.59%
  5406252 requests in 10.03s, 257.79MB read
  Socket errors: connect 0, read 137, write 4, timeout 24
Requests/sec: 538977.98
Transfer/sec:     25.70MB

normanmaurer · 2018-05-14T13:20:10Z

And @tanner0101

Lukasa

Some minor nits, and it'd be nice to have some testing of these behaviours, but otherwise fine.

Lukasa · 2018-05-14T13:31:05Z

Sources/NIOHTTP1/HTTPDecoder.swift

@@ -15,6 +15,14 @@
 import NIO
 import CNIOHTTPParser

+private extension UnsafeMutablePointer where Pointee == http_parser {
+


No need for this whitespace.

Lukasa · 2018-05-14T13:36:11Z

Sources/NIOHTTP1/HTTPTypes.swift

+private let connectionUtf8 = "connection".utf8
+
+
+// Keep state of keep alive state.


Nit: keep track, not keep state.

normanmaurer · 2018-05-14T13:45:28Z

@Lukasa yep will add some tests just to ensure we correctly set it to "unknown" when we add / remove connection header.

Motivation: To detect if keepalive is used we need to search the headers (if HTTP/1.1 is used). This may be expensive depending how many headers are present. http_parser itself detects already if keep alive should be used and so we can re-use this as long as the user did not modify the headers. Modifications: Reuse keepalive parsed by http_parser as long as the headers were not modified. Result: Less overhead as long as the headers are not modified.

normanmaurer · 2018-05-14T14:09:15Z

@Lukasa added some tests.

Lukasa

LGTM.

weissi · 2018-05-14T15:28:50Z

Sources/NIOHTTP1/HTTPTypes.swift

+        case .keepAlive:
+            return true
+        case .unknown:
+            guard let connection = self["connection"].first?.lowercased() else {


whilst you're working on this: worth fixing this implementation of follow-up?

Let me do a follow up

weissi

thanks! LGTM, just one question in there,.

normanmaurer requested review from Lukasa and weissi and removed request for Lukasa April 10, 2018 17:25

BasThomas reviewed Apr 10, 2018

View reviewed changes

normanmaurer force-pushed the use_parser_keepalive branch from 4362e38 to 3e6ecc0 Compare April 10, 2018 18:30

Lukasa requested changes Apr 11, 2018

View reviewed changes

normanmaurer force-pushed the use_parser_keepalive branch from 3e6ecc0 to 9fd09f5 Compare April 11, 2018 12:22

BasThomas reviewed Apr 11, 2018

View reviewed changes

normanmaurer force-pushed the use_parser_keepalive branch from 9fd09f5 to 7c150b0 Compare April 11, 2018 12:36

Lukasa requested changes Apr 11, 2018

View reviewed changes

normanmaurer force-pushed the use_parser_keepalive branch from 7c150b0 to a3923da Compare April 11, 2018 13:26

weissi requested changes Apr 11, 2018

View reviewed changes

normanmaurer force-pushed the use_parser_keepalive branch from a3923da to 90ef967 Compare April 11, 2018 18:19

Lukasa requested changes Apr 12, 2018

View reviewed changes

normanmaurer force-pushed the use_parser_keepalive branch from 90ef967 to ab4fb9b Compare April 12, 2018 11:58

Lukasa requested changes Apr 12, 2018

View reviewed changes

Lukasa added the semver/patch No public API change. label Apr 12, 2018

normanmaurer force-pushed the use_parser_keepalive branch from ab4fb9b to 0561283 Compare April 24, 2018 18:18

normanmaurer force-pushed the use_parser_keepalive branch from 0561283 to 39ed307 Compare May 14, 2018 13:17

Lukasa requested changes May 14, 2018

View reviewed changes

normanmaurer force-pushed the use_parser_keepalive branch from 39ed307 to a15b13f Compare May 14, 2018 13:47

normanmaurer force-pushed the use_parser_keepalive branch from a15b13f to ec97a92 Compare May 14, 2018 14:08

Lukasa approved these changes May 14, 2018

View reviewed changes

Lukasa added this to the 1.7.0 milestone May 14, 2018

weissi reviewed May 14, 2018

View reviewed changes

weissi approved these changes May 14, 2018

View reviewed changes

normanmaurer merged commit a7614d9 into apple:master May 14, 2018

normanmaurer deleted the use_parser_keepalive branch May 14, 2018 16:04

		private let connectionUtf8 = "connection".utf8


		// Keep state of keep alive state.

Reuse detected keepalive of http_parser as long as possible #299

Reuse detected keepalive of http_parser as long as possible #299

Conversation

normanmaurer commented Apr 10, 2018

normanmaurer commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented Apr 11, 2018

BasThomas Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented Apr 11, 2018

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Apr 12, 2018

Choose a reason for hiding this comment

normanmaurer commented Apr 12, 2018

weissi commented Apr 27, 2018

normanmaurer commented Apr 27, 2018

weissi commented Apr 27, 2018

tomerd commented Apr 30, 2018

normanmaurer commented May 14, 2018

normanmaurer commented May 14, 2018

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented May 14, 2018

normanmaurer commented May 14, 2018

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

BasThomas Apr 11, 2018 •

edited

Loading