Skip to content

Commit

Permalink
Reduce ARC in HPACK closestMatch (#268)
Browse files Browse the repository at this point in the history
Motivation:

When analysing remaining performance cliffs, we observed that
HeaderTableStorage.closestMatch was the source of a surprising amount of
ARC traffic. Further diagnosis revealed that we were ARCing string
storage as we iterated the table. Given that the table is constructed of
String/String pairs, the effect would be that if we scanned a table we
would emit two retains and two releases for each entry. Whenever we
encode a HTTP/2 header that we think could be indexed, we will scan the
static and possibly the dynamic tables to find a header that matches.
This ends up being a pretty nasty performance cost, as swift_retain and
swift_release are particularly complex on Strings, and often have
trouble with cache coherency and cache misses.

This got root-caused to https://bugs.swift.org/browse/SR-13931.

Modifications:

- Work around SR-13931 by avoiding using the iterator.
- While we're here, remove generics as well, as we aren't using them.

Results:

4% perf gain in 10k requests 100 concurrent streams benchmark and
10k requests 1 concurrent streams benchmark.
  • Loading branch information
Lukasa authored Dec 4, 2020
1 parent f4b49cb commit 1dd123a
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 13 deletions.
12 changes: 4 additions & 8 deletions Sources/NIOHPACK/HPACKHeader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -456,16 +456,12 @@ private extension Substring {
}


extension Sequence where Self.Element == UInt8 {
/// Compares the collection of `UInt8`s to a case insensitive collection.
///
/// This collection could be get from applying the `UTF8View`
/// property on the string protocol.
extension String.UTF8View {
/// Compares two UTF8 strings as case insensitive ASCII bytes.
///
/// - Parameter bytes: The string constant in the form of a collection of `UInt8`
/// - Returns: Whether the collection contains **EXACTLY** this array or no, but by ignoring case.
fileprivate func compareCaseInsensitiveASCIIBytes<T: Sequence>(to other: T) -> Bool
where T.Element == UInt8 {
fileprivate func compareCaseInsensitiveASCIIBytes(to other: String.UTF8View) -> Bool {
// fast path: we can get the underlying bytes of both
let maybeMaybeResult = self.withContiguousStorageIfAvailable { lhsBuffer -> Bool? in
other.withContiguousStorageIfAvailable { rhsBuffer in
Expand All @@ -491,7 +487,7 @@ extension Sequence where Self.Element == UInt8 {
}

@inline(never)
private func _compareCaseInsensitiveASCIIBytesSlowPath<T: Sequence>(to other: T) -> Bool where T.Element == UInt8 {
private func _compareCaseInsensitiveASCIIBytesSlowPath(to other: String.UTF8View) -> Bool {
return self.elementsEqual(other, by: { return (($0 & 0xdf) == ($1 & 0xdf) && $0.isASCII) })
}
}
Expand Down
24 changes: 19 additions & 5 deletions Sources/NIOHPACK/HeaderTables.swift
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,32 @@ struct HeaderTableStorage {
func closestMatch(name: String, value: String) -> MatchType {
var partialIndex: Int? = nil

for (index, header) in self.headers.enumerated() {
// Yes, I'm manually reimplementing IndexingIterator here. This is because
// the excess ARC in this loop shows up in our profiles pretty substantially,
// and it's triggered by https://bugs.swift.org/browse/SR-13931.
//
// Working around this until the above is resolved.
var offset = 0
var index = self.headers.startIndex

while index < self.headers.endIndex {
defer {
// Unchecked arithmetic is safe here, we can't overflow as offset can never exceed count.
offset &+= 1
self.headers.formIndex(after: &index)
}

// Check if the header name matches.
guard header.name.isEqualCaseInsensitiveASCIIBytes(to: name) else {
guard self.headers[index].name.isEqualCaseInsensitiveASCIIBytes(to: name) else {
continue
}

if partialIndex == nil {
partialIndex = index
partialIndex = offset
}

if value == header.value {
return .full(index)
if value == self.headers[index].value {
return .full(offset)
}
}

Expand Down

0 comments on commit 1dd123a

Please sign in to comment.