-
Notifications
You must be signed in to change notification settings - Fork 113
Small Performance Improvements #199
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.
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
Small Performance Improvements #199
Conversation
var version: HTTPVersion | ||
var status: HTTPResponseStatus | ||
var headers: HTTPHeaders | ||
var body: ByteBuffer? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cleanup change. public
access control in a internal struct
doesn't make much sense.
case .body(let head, var body): | ||
body.writeBuffer(&bodyPart) | ||
self.readState = .body(head, body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a fairly large performance issue here... Since body
's internal storage is referenced from self.readState
and from body
, we incur a CoW on every append to the local buffer. We could fix this by an easy CoW prevention like:
case .body(let head, var body):
self.readState = .idle // removes the self.readState reference. Therefore no Cow necessary
body.writeBuffer(&bodyPart)
self.readState = .body(head, body)
However since October of last year NIO has a NIOHTTPClientResponseAggregator
. I think we should use that instead. All the http write logic from HTTPHandler
has been moved into UnaryHandler
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moving to NIOHTTPClientResponseAggregator
makes perfect sense. I wonder tho if moving all the extra functionality to UnaryHandler
is overloading its intent - to be a unary handler. not sure the performance costs of having another handlers, if low, we can consider splitting that up, alternatively we can rename UnaryHandler
to something more appropriate givens expanded role
@@ -64,7 +64,6 @@ extension Lambda { | |||
struct RuntimeEngine: CustomStringConvertible { | |||
let ip: String | |||
let port: Int | |||
let keepAlive: Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the keepAlive
option here, since neither tested nor used anywhere in the project. The HTTPClient should always prefer keep-alive
but should be able to close a connection after a connection: close
header. There is a test that ensures that the HTTPClient works correctly when a connection: close
header is received.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intent was to be able and force closed connection by sending that header from the client, but this is fine.
@@ -30,7 +30,7 @@ internal final class MockLambdaServer { | |||
private var shutdown = false | |||
|
|||
public init(behavior: LambdaServerBehavior, host: String = "127.0.0.1", port: Int = 7000, keepAlive: Bool = true) { | |||
self.group = MultiThreadedEventLoopGroup(numberOfThreads: System.coreCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes Xcodes debug view much easier to deal with! And we only need a single core for the MockServer anyway.
03dfd03
to
3a95db1
Compare
3a95db1
to
d0a15da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good improvement here. one dilemma regarding the expanded role of UnaryHandler
to decide before we merge
d0a15da
to
9a64ed9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
This PR has a number of smaller perf improvements. In my measuring they account for about a 3% perf improvement with tiny string payloads. The perf improvement should be much larger for larger invocation payloads.
Modifications:
HTTPHandler
withNIOHTTPClientResponseAggregator
, to fix a CoW issue. This will have by far the biggest speed impact. Especially for large payloads.EventLoop
in tests.syncOperations
to setup theHTTPClient
.Result:
Faster Swift Lambda Runtime.