From 1dd123a849812386f8d6beb22f9728fc515a0021 Mon Sep 17 00:00:00 2001
From: Cory Benfield <lukasa@apple.com>
Date: Fri, 4 Dec 2020 09:43:24 +0000
Subject: [PATCH] Reduce ARC in HPACK closestMatch (#268)

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.
---
 Sources/NIOHPACK/HPACKHeader.swift  | 12 ++++--------
 Sources/NIOHPACK/HeaderTables.swift | 24 +++++++++++++++++++-----
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/Sources/NIOHPACK/HPACKHeader.swift b/Sources/NIOHPACK/HPACKHeader.swift
index 673c8374..ed270a23 100644
--- a/Sources/NIOHPACK/HPACKHeader.swift
+++ b/Sources/NIOHPACK/HPACKHeader.swift
@@ -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
@@ -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) })
     }
 }
diff --git a/Sources/NIOHPACK/HeaderTables.swift b/Sources/NIOHPACK/HeaderTables.swift
index db0500d9..a31cf02d 100644
--- a/Sources/NIOHPACK/HeaderTables.swift
+++ b/Sources/NIOHPACK/HeaderTables.swift
@@ -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)
             }
         }