-
Notifications
You must be signed in to change notification settings - Fork 652
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
Do not allocate String to check if HTTP message is keep alive #402
Conversation
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.
like it because the .lowercased
was always annoying and a waste.
Sources/NIOHTTP1/HTTPTypes.swift
Outdated
|
||
if connection == "close" { | ||
return false | ||
if !self.headers.isEmpty { |
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 seems superfluous as you iterate self.headers
just after that. If this is empty, the loop won't do anything either.
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.
ok :)
Sources/NIOHTTP1/HTTPTypes.swift
Outdated
} | ||
return connection == "keep-alive" | ||
// HTTP 1.1 use keep-alive by default if not otherwise told. | ||
return version.major == 1 && version.minor == 1 |
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 should be version.minor >= 1
as RFC says to treat 1.x where x>=1 like 1.1
Sources/NIOHTTP1/HTTPTypes.swift
Outdated
@@ -18,6 +18,8 @@ let crlf: StaticString = "\r\n" | |||
let headerSeparator: StaticString = ": " | |||
|
|||
private let connectionUtf8 = "connection".utf8 | |||
private let closeUtf8 = "close".utf8 |
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 these global variables do nothing (in the good case) and cause a slowdown if the compiler isn't able to optimise it. Just put them where you use them
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.
@weissi hmm I would have thought its actually better as .utf8
should only be called once ? Maybe I should make these static ?
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.
@normanmaurer .utf8
is a function that needs to be inlined. If you want to cache the bytes straight, you'll need to do Array("close".utf8)
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.
ah ok... then let me just do what you said first.
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.
that should be pretty fast in 4.1 and really fast in 4.2
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.
only using StaticString
s would be faster but we unfortunately don't support them yet for headers (which we should fix)
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.
@weissi on my to do list for this week ;)
Sources/NIOHTTP1/HTTPTypes.swift
Outdated
if !self.headers.isEmpty { | ||
for header in self.headers { | ||
if self.buffer.equalCaseInsensitiveASCII(view: connectionUtf8, at: header.name) { | ||
if self.buffer.equalCaseInsensitiveASCII(view: closeUtf8, at: header.value) { |
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.
technically this isn't a correct implementation, right? could be connection: keep-alive, foobar
no?
I know, it's not worse than the previous implementation either but the fast path is anyway that there is no connection
header
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.
@@ -18,6 +18,8 @@ let crlf: StaticString = "\r\n" | |||
let headerSeparator: StaticString = ": " |
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.
please change the allocation counter test limits and reduce according to the number of allocs saved
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.
@weissi let me see if it changes anything (as it will only if we actual have a connection header).
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.
return false | ||
for header in self.headers { | ||
if self.buffer.equalCaseInsensitiveASCII(view: "connection".utf8, at: header.name) { | ||
if self.buffer.equalCaseInsensitiveASCII(view: "close".utf8, at: header.value) { |
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 don't think this is quite right, though to be fair this has never been quite right. Strictly speaking I think this will fail if the header is Connection: close, some-other-header-name
, when it should not. Additionally, neither close
nor keep-alive
strictly have to be first.
Do we want to add some tolerance for that, or set it as a contributor friendly issue and let the community tackle it?
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.
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.
@Lukasa yes, commented on that one too but I'd be happy with a follow-up.
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.
will do a followup
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.
🙌
@Lukasa cool with me merging ? |
Motivation: We can check if a HTTP message is using keep alive without any allocation. Modifications: Directly act on the internal storage of HTTPHeaders to find out if keep alive is used or not. Result: Less allocations
152d300
to
0210df4
Compare
Motivation:
We can check if a HTTP message is using keep alive without any allocation.
Modifications:
Directly act on the internal storage of HTTPHeaders to find out if keep alive is used or not.
Result:
Less allocations