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

Optimize for HTTP/1.1 and HTTP/1.0 responses #300

Merged
merged 3 commits into from Apr 12, 2018

Conversation

Projects
None yet
3 participants
@normanmaurer
Copy link
Member

normanmaurer commented Apr 11, 2018

Motivation:

Its very likely that the response will be HTTP/1.1 or HTTP/1.0 so we can optimize for it by minimizing the buffer.write(...) calls.
Beside this we should also change the pattern of *.write(into: inout ByteBuffer) to extension on ByteBuffer itself.

Modificiations:

- Optimize for HTTP/1.0 and 1.1
- Use extensions on ByteBuffer

Result:

Faster and more clean code.
@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Apr 11, 2018

Before:

Requests/sec: 105724.17
Transfer/sec:      5.04MB

With this PR:

Requests/sec: 114734.71
Transfer/sec:      5.47MB

@normanmaurer normanmaurer requested review from Lukasa and weissi Apr 11, 2018

@normanmaurer normanmaurer force-pushed the normanmaurer:encoder_optimize branch from 2bf5b7d to 76efe8c Apr 11, 2018

@weissi

weissi approved these changes Apr 11, 2018

Copy link
Contributor

weissi left a comment

lgtm

self.write(staticString: status200)
} else {
self.write(string: String(status.code))
self.write(string: " ")

This comment has been minimized.

@Lukasa

Lukasa Apr 11, 2018

Contributor

This should be staticString.

This comment has been minimized.

@weissi

weissi Apr 11, 2018

Contributor

@Lukasa we should really probably make this self.write(bytes: [32]) or self.write(integer: 32, as: UInt8.self). No idea what's fastest.

This comment has been minimized.

@weissi

weissi Apr 11, 2018

Contributor

@Lukasa for one character I'm also not sure if staticString is actually faster than string. No idea

case .UNSUBSCRIBE:
self.write(staticString: "UNSUBSCRIBE")
case .RAW(let value):
self.write(string: value)

This comment has been minimized.

@Lukasa

Lukasa Apr 11, 2018

Contributor

Why aren't you making this optimisation for all the status codes too?

This comment has been minimized.

@normanmaurer

normanmaurer Apr 11, 2018

Author Member

done...

@normanmaurer normanmaurer force-pushed the normanmaurer:encoder_optimize branch from 76efe8c to aafe73c Apr 11, 2018

@@ -58,7 +58,7 @@ private func writeTrailers(wrapOutboundOut: (IOData) -> NIOAny, ctx: ChannelHand
if let trailers = trailers {
buffer = ctx.channel.allocator.buffer(capacity: 256)
buffer.write(staticString: "0\r\n")
trailers.write(into: &buffer) // Includes trailing CRLF.
buffer.write(headers: trailers)

This comment has been minimized.

@Lukasa

Lukasa Apr 11, 2018

Contributor

Can you leave the old comment here?

This comment has been minimized.

@normanmaurer

normanmaurer Apr 11, 2018

Author Member

sure...

// Optimize for HTTP/1.0 and HTTP/1.1
if response.version.major == 1 {
if response.version.minor == 1 {
switch response.status {

This comment has been minimized.

@Lukasa

Lukasa Apr 11, 2018

Contributor

Is this better written as a single switch statement, rather than two? E.g. switch (response.version.major, response.version.minor, status)?

This comment has been minimized.

@normanmaurer

normanmaurer Apr 11, 2018

Author Member

Yes! TIL :)

if version.major == 1 && version.minor == 1 {
// Optimize for HTTP/1.1
self.write(staticString: http1_1)
} else {

This comment has been minimized.

@Lukasa

Lukasa Apr 11, 2018

Contributor

This could also specialise HTTP/1.0.

private mutating func write(status: HTTPResponseStatus) {
if case .ok = status {
// Optimize for 200 ok, which should be the most likely code (...hopefully).
self.write(staticString: status200)

This comment has been minimized.

@Lukasa

Lukasa Apr 11, 2018

Contributor

Do we actually need this? I don't think we'll ever enter this branch, because it requires 1.2 or higher to get to.

This comment has been minimized.

@normanmaurer

normanmaurer Apr 11, 2018

Author Member

I think this is safe to remove :)

@normanmaurer normanmaurer force-pushed the normanmaurer:encoder_optimize branch 2 times, most recently from f9a2d6f to 4821b1a Apr 11, 2018

@normanmaurer normanmaurer changed the title Optimize for HTTP/1.1 OK responses Optimize for HTTP/1.0 and HTTP/1.0 responses Apr 11, 2018

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Apr 11, 2018

@Lukasa PTAL again

@normanmaurer normanmaurer changed the title Optimize for HTTP/1.0 and HTTP/1.0 responses Optimize for HTTP/1. and HTTP/1.0 responses Apr 12, 2018

@normanmaurer normanmaurer force-pushed the normanmaurer:encoder_optimize branch from 4821b1a to 2f7dba7 Apr 12, 2018

@normanmaurer normanmaurer changed the title Optimize for HTTP/1. and HTTP/1.0 responses Optimize for HTTP/1.1 and HTTP/1.0 responses Apr 12, 2018

@Lukasa
Copy link
Contributor

Lukasa left a comment

You're not going to like this feedback, sorry!

case (1, 0, .resetContent):
self.write(staticString: "HTTP/1.0 205 Reset Content\r\n")
case (1, 0, .partialContent):
self.write(staticString: "HTTP/1.0 206 Partital Content\r\n")

This comment has been minimized.

@Lukasa

Lukasa Apr 12, 2018

Contributor

Typo: partial.

case (1, 0, .multiStatus):
self.write(staticString: "HTTP/1.0 207 Multi-Status\r\n")
case (1, 0, .alreadyReported):
self.write(staticString: "HTTP/1.0 208 IM Already Reported\r\n")

This comment has been minimized.

@Lukasa

Lukasa Apr 12, 2018

Contributor

No "IM" here.

case (1, 0, .multipleChoices):
self.write(staticString: "HTTP/1.0 300 Multiple Choices\r\n")
case (1, 0, .movedPermanently):
self.write(staticString: "HTTP/1.0 301 Multiple Choices\r\n")

This comment has been minimized.

@Lukasa

Lukasa Apr 12, 2018

Contributor

301 is Moved Permanently

case (1, 0, .movedPermanently):
self.write(staticString: "HTTP/1.0 301 Multiple Choices\r\n")
case (1, 0, .found):
self.write(staticString: "HTTP/1.0 302 Multiple Choices\r\n")

This comment has been minimized.

@Lukasa

Lukasa Apr 12, 2018

Contributor

302 is Found

case (1, 0, .forbidden):
self.write(staticString: "HTTP/1.0 403 Forbidden\r\n")
case (1, 0, .notFound):
self.write(staticString: "HTTP/1.0 404 Forbidden\r\n")

This comment has been minimized.

@Lukasa

Lukasa Apr 12, 2018

Contributor

404 is Not Found.

case (1, 1, .movedPermanently):
self.write(staticString: "HTTP/1.1 301 Multiple Choices\r\n")
case (1, 1, .found):
self.write(staticString: "HTTP/1.1 302 Multiple Choices\r\n")

This comment has been minimized.

@Lukasa

Lukasa Apr 12, 2018

Contributor

302 is Found.

case (1, 1, .forbidden):
self.write(staticString: "HTTP/1.1 403 Forbidden\r\n")
case (1, 1, .notFound):
self.write(staticString: "HTTP/1.1 404 Forbidden\r\n")

This comment has been minimized.

@Lukasa

Lukasa Apr 12, 2018

Contributor

404 is Not Found.

case (1, 1, .preconditionFailed):
self.write(staticString: "HTTP/1.1 412 Precondition Failed\r\n")
case (1, 1, .payloadTooLarge):
self.write(staticString: "HTTP/1.1 413 Request Entity Too Large\r\n")

This comment has been minimized.

@Lukasa

Lukasa Apr 12, 2018

Contributor

413 is Payload Too Large.

case (1, 1, .payloadTooLarge):
self.write(staticString: "HTTP/1.1 413 Request Entity Too Large\r\n")
case (1, 1, .uriTooLong):
self.write(staticString: "HTTP/1.1 414 Request-URI Too Long\r\n")

This comment has been minimized.

@Lukasa

Lukasa Apr 12, 2018

Contributor

414 is URI Too Long

case (1, 1, .unsupportedMediaType):
self.write(staticString: "HTTP/1.1 415 Unsupported Media Type\r\n")
case (1, 1, .rangeNotSatisfiable):
self.write(staticString: "HTTP/1.1 416 Request Range Not Satisified\r\n")

This comment has been minimized.

@Lukasa

Lukasa Apr 12, 2018

Contributor

416 is Range Not Satisfiable

Optimize for HTTP/1.1 and HTTP/1.0 responses
Motivation:

Its very likely that the response will be HTTP/1.1 or HTTP/1.0 so we can optimize for it by minimizing the buffer.write(...) calls.
Beside this we should also change the pattern of *.write(into: inout ByteBuffer) to extension on ByteBuffer itself.

Modificiations:

- Optimize for HTTP/1.0 and 1.1
- Use extensions on ByteBuffer

Result:

Faster and more clean code.

@normanmaurer normanmaurer force-pushed the normanmaurer:encoder_optimize branch from 2f7dba7 to aa2f1fa Apr 12, 2018

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Apr 12, 2018

@Lukasa addressed

@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Apr 12, 2018

@swift-nio-bot test this please

@Lukasa

Lukasa approved these changes Apr 12, 2018

Copy link
Contributor

Lukasa left a comment

Ok, happy for this to land when the tests are passing.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Apr 12, 2018

ok passed... rebasing and will merge :)

@normanmaurer normanmaurer added this to the 1.5.0 milestone Apr 12, 2018

@normanmaurer normanmaurer merged commit 575e768 into apple:master Apr 12, 2018

1 of 2 checks passed

pull request validation (4.1) Build started sha1 is merged.
Details
pull request validation (4.0.3) Build finished.
Details

@normanmaurer normanmaurer deleted the normanmaurer:encoder_optimize branch Apr 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment