Move to String-backed HPACK representations. (#89)

Motivation:

When originally written our HPACK implementation targetted Swift 4.2,
which had performance issues when storing mostly-ASCII data in Strings.
For this reason we had a number of data types that used ByteBuffers as
their backing storage and reified Strings when necessary.

In Swift 5 this is no longer worthwhile: in fact, it imposes performance
penalties above and beyond simply using the Strings. For this reason, we
should move to a simpler model where we simply store Strings again.

Modifications:

- Rewrote all HPACK data structures to use Strings as their backing store.
- Removed now-redundant genericism.
- Removed now-unnecessary StringRing data structure and associated tests.

Result:

Faster performance in almost all cases.

Sources/NIOHPACK/DynamicHeaderTable.swift

@@ -57,8 +57,8 @@ struct DynamicHeaderTable {
         return self.storage.count
     }
 
-    init(allocator: ByteBufferAllocator, maximumLength: Int = DynamicHeaderTable.defaultSize) {
-        self.storage = HeaderTableStorage(allocator: allocator, maxSize: maximumLength)
+    init(maximumLength: Int = DynamicHeaderTable.defaultSize) {
+        self.storage = HeaderTableStorage(maxSize: maximumLength)
         self.maximumTableLength = maximumLength
         self.allowedLength = maximumLength  // until we're told otherwise, this is what we assume the other side expects.
     }
@@ -68,10 +68,6 @@ struct DynamicHeaderTable {
         return self.storage[i]
     }
 
-    func view(of index: HPACKHeaderIndex) -> ByteBufferView {
-        return self.storage.view(of: index)
-    }
-
     // internal for testing
     func dumpHeaders() -> String {
         return self.storage.dumpHeaders(offsetBy: StaticHeaderTable.count)
@@ -94,7 +90,7 @@ struct DynamicHeaderTable {
     /// - Returns: A tuple containing the matching index and, if a value was specified as a
     ///            parameter, an indication whether that value was also found. Returns `nil`
     ///            if no matching header name could be located.
-    func findExistingHeader<Name: Collection, Value: Collection>(named name: Name, value: Value?) -> (index: Int, containsValue: Bool)? where Name.Element == UInt8, Value.Element == UInt8 {
+    func findExistingHeader(named name: String, value: String?) -> (index: Int, containsValue: Bool)? {
         // looking for both name and value, but can settle for just name if no value
         // has been provided. Return the first matching name (lowest index) in that case.
         guard let value = value else {
@@ -124,16 +120,17 @@ struct DynamicHeaderTable {
     /// > emptied of all existing entries and results in an empty table."
     ///
     /// - Parameters:
-    ///   - name: A collection of UTF-8 code points comprising the name of the header to insert.
-    ///   - value: A collection of UTF-8 code points comprising the value of the header to insert.
+    ///   - name: A String representing the name of the header field.
+    ///   - value: A String representing the value of the header field.
     /// - Returns: `true` if the header was added to the table, `false` if not.
-    mutating func addHeader<Name: Collection, Value: Collection>(named name: Name, value: Value) throws where Name.Element == UInt8, Value.Element == UInt8 {
+    mutating func addHeader(named name: String, value: String) throws {
         do {
             try self.storage.add(name: name, value: value)
-        } catch let error as RingBufferError.BufferOverrun {
+        } catch InternalError.unableToAddHeaderToTable(let excess) {
             // ping the error up the stack, with more information
-            throw NIOHPACKErrors.FailedToAddIndexedHeader(bytesNeeded: self.storage.length + error.amount,
-                                                          name: name, value: value)
+            // TODO(cory): Remove the genericism here in future.
+            throw NIOHPACKErrors.FailedToAddIndexedHeader(bytesNeeded: self.storage.length + excess,
+                                                          name: name.utf8, value: value.utf8)
         }
     }
 }

Sources/NIOHPACK/HPACKDecoder.swift

@@ -33,8 +33,6 @@ public struct HPACKDecoder {
     // private but tests
     var headerTable: IndexedHeaderTable
 
-    private var decodeBuffer: ByteBuffer
-
     var dynamicTableLength: Int {
         return headerTable.dynamicTableLength
     }
@@ -76,10 +74,6 @@ public struct HPACKDecoder {
     /// - Parameter maxDynamicTableSize: Maximum allowed size of the dynamic header table.
     public init(allocator: ByteBufferAllocator, maxDynamicTableSize: Int = HPACKDecoder.maxDynamicTableSize) {
         self.headerTable = IndexedHeaderTable(allocator: allocator, maxDynamicTableSize: maxDynamicTableSize)
-
-        // start the decode buffer at the requested size of the dynamic header list table, or the default
-        // size, whichever is larger.
-        self.decodeBuffer = allocator.buffer(capacity: max(maxDynamicTableSize, DynamicHeaderTable.defaultSize))
     }
 
     /// Reads HPACK encoded header data from a `ByteBuffer`.
@@ -94,21 +88,35 @@ public struct HPACKDecoder {
     public mutating func decodeHeaders(from buffer: inout ByteBuffer) throws -> HPACKHeaders {
         // take a local copy to mutate
         var bufCopy = buffer
-        self.decodeBuffer.clear()
-
         var headers: [HPACKHeader] = []
+
         while bufCopy.readableBytes > 0 {
-            // this will update our own `decodeBuffer` property.
-            if let header = try self.decodeHeader(from: &bufCopy) {
+            switch try self.decodeHeader(from: &bufCopy) {
+            case .tableSizeChange:
+                // RFC 7541 § 4.2 <https://httpwg.org/specs/rfc7541.html#maximum.table.size>:
+                //
+                // 2. "This dynamic table size update MUST occur at the beginning of the first header block
+                //    following the change to the dynamic table size."
+                guard headers.count == 0 else {
+                    // If our decode buffer has any data in it, then this is out of place.
+                    // Treat it as an invalid input
+                    throw NIOHPACKErrors.IllegalDynamicTableSizeChange()
+                }
+            case .header(let header):
                 headers.append(header)
             }
         }
 
         buffer = bufCopy
-        return HPACKHeaders(buffer: self.decodeBuffer, headers: headers)
+        return HPACKHeaders(headers: headers)
     }
-
-    private mutating func decodeHeader(from buffer: inout ByteBuffer) throws -> HPACKHeader? {
+
+    enum DecodeResult {
+        case tableSizeChange
+        case header(HPACKHeader)
+    }
+
+    private mutating func decodeHeader(from buffer: inout ByteBuffer) throws -> DecodeResult {
         // peek at the first byte to decide how many significant bits of that byte we
         // will need to include in our decoded integer. Some values use a one-bit prefix,
         // some use two, or four.
@@ -123,21 +131,21 @@ public struct HPACKDecoder {
             guard hidx != 0 else {
                 throw NIOHPACKErrors.ZeroHeaderIndex()
             }
-            return try self.decodeIndexedHeader(from: Int(hidx))
+            return try .header(self.decodeIndexedHeader(from: Int(hidx)))
 
         case let x where x & 0b1100_0000 == 0b0100_0000:
             // 0b01xxxxxx -- two-bit prefix, six bits of value
             // literal header with possibly-indexed name
             let hidx = try buffer.readEncodedInteger(withPrefix: 6)
-            return try self.decodeLiteralHeader(from: &buffer, headerName: HPACKString(fromEncodedInteger: hidx), addToIndex: true)
+            return try .header(self.decodeLiteralHeader(from: &buffer, headerName: HPACKString(fromEncodedInteger: hidx), addToIndex: true))
 
         case let x where x & 0b1111_0000 == 0b0000_0000:
             // 0b0000xxxx -- four-bit prefix, four bits of value
             // literal header with possibly-indexed name, not added to dynamic table
             let hidx = try buffer.readEncodedInteger(withPrefix: 4)
             var header = try self.decodeLiteralHeader(from: &buffer, headerName: HPACKString(fromEncodedInteger: hidx), addToIndex: false)
             header.indexing = .nonIndexable
-            return header
+            return .header(header)
 
         case let x where x & 0b1111_0000 == 0b0001_0000:
             // 0b0001xxxx -- four-bit prefix, four bits of value
@@ -146,74 +154,54 @@ public struct HPACKDecoder {
             let hidx = try buffer.readEncodedInteger(withPrefix: 4)
             var header = try self.decodeLiteralHeader(from: &buffer, headerName: HPACKString(fromEncodedInteger: hidx), addToIndex: false)
             header.indexing = .neverIndexed
-            return header
+            return .header(header)
 
         case let x where x & 0b1110_0000 == 0b0010_0000:
             // 0b001xxxxx -- three-bit prefix, five bits of value
             // dynamic header table size update
             let newMaxLength = try Int(buffer.readEncodedInteger(withPrefix: 5))
 
-            // RFC 7541 § 4.2 <https://httpwg.org/specs/rfc7541.html#maximum.table.size> gives us two conditions:
+            // RFC 7541 § 4.2 <https://httpwg.org/specs/rfc7541.html#maximum.table.size>:
             //
             // 1. "the chosen size MUST stay lower than or equal to the maximum set by the protocol."
             guard newMaxLength <= self.headerTable.maxDynamicTableLength else {
                 throw NIOHPACKErrors.InvalidDynamicTableSize(requestedSize: newMaxLength, allowedSize: self.headerTable.maxDynamicTableLength)
             }
 
-            // 2. "This dynamic table size update MUST occur at the beginning of the first header block
-            //    following the change to the dynamic table size."
-            guard self.decodeBuffer.readableBytes == 0 else {
-                // If our decode buffer has any data in it, then this is out of place.
-                // Treat it as an invalid input
-                throw NIOHPACKErrors.IllegalDynamicTableSizeChange()
-            }
-
             self.allowedDynamicTableLength = newMaxLength
-            return nil
+            return .tableSizeChange
 
         default:
             throw NIOHPACKErrors.InvalidHeaderStartByte(byte: initial)
         }
     }
 
     private mutating func decodeIndexedHeader(from hidx: Int) throws -> HPACKHeader {
-        let (h, v) = try self.headerTable.headerViews(at: hidx)
-
-        let start = self.decodeBuffer.writerIndex
-        let nlen = self.decodeBuffer.writeBytes(h)
-        let vlen = self.decodeBuffer.writeBytes(v)
-
-        return HPACKHeader(start: start, nameLength: nlen, valueLength: vlen)
+        let (name, value) = try self.headerTable.header(at: hidx)
+        return HPACKHeader(name: name, value: value)
     }
 
     private mutating func decodeLiteralHeader(from buffer: inout ByteBuffer, headerName: HPACKString,
                                               addToIndex: Bool) throws -> HPACKHeader {
-        let nameStart = self.decodeBuffer.writerIndex
-        let nameLen: Int
-        let valueLen: Int
+        let name: String
 
         switch headerName {
         case .indexed(let idx):
-            let (name, _) = try self.headerTable.headerViews(at: idx)
-            nameLen = self.decodeBuffer.writeBytes(name)
+            (name, _) = try self.headerTable.header(at: idx)
         case .literal:
-            nameLen = try self.readEncodedString(from: &buffer)
+            name = try self.readEncodedString(from: &buffer)
         }
 
-        valueLen = try self.readEncodedString(from: &buffer)
+        let value = try self.readEncodedString(from: &buffer)
 
         if addToIndex {
-            // These views are safe to force-unwrap as we have just written these bytes into the decode buffer.
-            let nameView = self.decodeBuffer.viewBytes(at: nameStart, length: nameLen)!
-            let valueView = self.decodeBuffer.viewBytes(at: nameStart + nameLen, length: valueLen)!
-
-            try headerTable.add(headerNamed: nameView, value: valueView)
+            try headerTable.add(headerNamed: name, value: value)
         }
 
-        return HPACKHeader(start: nameStart, nameLength: nameLen, valueLength: valueLen)
+        return HPACKHeader(name: name, value: value)
     }
 
-    private mutating func readEncodedString(from buffer: inout ByteBuffer) throws -> Int {
+    private mutating func readEncodedString(from buffer: inout ByteBuffer) throws -> String {
         // peek to read the encoding bit
         guard let initialByte: UInt8 = buffer.getInteger(at: buffer.readerIndex) else {
             throw NIOHPACKErrors.InsufficientInput()
@@ -225,16 +213,12 @@ public struct HPACKDecoder {
         guard len <= buffer.readableBytes else {
             throw NIOHPACKErrors.StringLengthBeyondPayloadSize(length: len, available: buffer.readableBytes)
         }
-
-        let outputLength: Int
+
         if huffmanEncoded {
-            outputLength = try buffer.getHuffmanEncodedString(at: buffer.readerIndex, length: len, into: &self.decodeBuffer)
+            return try buffer.readHuffmanEncodedString(length: len)
         } else {
             // This force-unwrap is safe as we have checked above that len is less than or equal to the buffer readable bytes.
-            outputLength = self.decodeBuffer.writeBytes(buffer.viewBytes(at: buffer.readerIndex, length: len)!)
+           return buffer.readString(length: len)!
         }
-
-        buffer.moveReaderIndex(forwardBy: len)
-        return outputLength
     }
 }

Sources/NIOHPACK/HPACKEncoder.swift

@@ -205,11 +205,11 @@ public struct HPACKEncoder {
         for header in headers {
             switch header.indexing {
             case .indexable:
-                try self._appendIndexed(header: header.name.utf8, value: header.value.utf8)
+                try self._appendIndexed(header: header.name, value: header.value)
             case .nonIndexable:
-                self._appendNonIndexed(header: header.name.utf8, value: header.value.utf8)
+                self._appendNonIndexed(header: header.name, value: header.value)
             case .neverIndexed:
-                self._appendNeverIndexed(header: header.name.utf8, value: header.value.utf8)
+                self._appendNeverIndexed(header: header.name, value: header.value)
             }
         }
     }
@@ -222,17 +222,14 @@ public struct HPACKEncoder {
             throw NIOHPACKErrors.EncoderNotStarted()
         }
 
-        for header in headers.headerIndices {
-            let nameView = headers.view(of: header.name)
-            let valueView = headers.view(of: header.value)
-
-            switch header.indexing {
+        for header in headers {
+            switch header.indexable {
             case .indexable:
-                try self._appendIndexed(header: nameView, value: valueView)
+                try self._appendIndexed(header: header.name, value: header.value)
             case .nonIndexable:
-                self._appendNonIndexed(header: nameView, value: valueView)
+                self._appendNonIndexed(header: header.name, value: header.value)
             case .neverIndexed:
-                self._appendNeverIndexed(header: nameView, value: valueView)
+                self._appendNeverIndexed(header: header.name, value: header.value)
             }
         }
     }
@@ -244,11 +241,11 @@ public struct HPACKEncoder {
         guard case .encoding = self.state else {
             throw NIOHPACKErrors.EncoderNotStarted()
         }
-        try self._appendIndexed(header: name.utf8, value: value.utf8)
+        try self._appendIndexed(header: name, value: value)
     }
 
     /// Returns `true` if the item needs to be added to the header table
-    private mutating func _appendIndexed<Name: Collection, Value: Collection>(header name: Name, value: Value) throws where Name.Element == UInt8, Value.Element == UInt8 {
+    private mutating func _appendIndexed(header name: String, value: String) throws {
         if let (index, hasValue) = self.headerIndexTable.firstHeaderMatch(for: name, value: value) {
             if hasValue {
                 // purely indexed. Nice & simple.
@@ -273,7 +270,9 @@ public struct HPACKEncoder {
         try self.headerIndexTable.add(headerNamed: name, value: value)
     }
 
-    private mutating func appendEncodedString<C: Collection>(_ utf8: C) where C.Element == UInt8 {
+    private mutating func appendEncodedString(_ string: String) {
+        let utf8 = string.utf8
+
         // encode the value
         if self.useHuffmanEncoding {
             // problem: we need to encode the length before the encoded bytes, so we can't just receive the length
@@ -292,10 +291,10 @@ public struct HPACKEncoder {
         guard case .encoding = self.state else {
             throw NIOHPACKErrors.EncoderNotStarted()
         }
-        self._appendNonIndexed(header: header.utf8, value: value.utf8)
+        self._appendNonIndexed(header: header, value: value)
     }
 
-    private mutating func _appendNonIndexed<Name: Collection, Value: Collection>(header: Name, value: Value) where Name.Element == UInt8, Value.Element == UInt8 {
+    private mutating func _appendNonIndexed(header: String, value: String) {
         if let (index, _) = self.headerIndexTable.firstHeaderMatch(for: header, value: value) {
             // we actually don't care if it has a value in this instance; we're only indexing the
             // name.
@@ -314,12 +313,11 @@ public struct HPACKEncoder {
         guard case .encoding = self.state else {
             throw NIOHPACKErrors.EncoderNotStarted()
         }
-        self._appendNeverIndexed(header: header.utf8, value: value.utf8)
+        self._appendNeverIndexed(header: header, value: value)
     }
 
-    private mutating func _appendNeverIndexed<Name: Collection, Value: Collection>(header: Name, value: Value) where Name.Element == UInt8, Value.Element == UInt8 {
-        // compiler doesn't like us passing `nil` here as it can't infer the types, apparently.
-        if let (index, _) = self.headerIndexTable.firstHeaderMatch(for: header, value: Optional<Value>.none) {
+    private mutating func _appendNeverIndexed(header: String, value: String) {
+        if let (index, _) = self.headerIndexTable.firstHeaderMatch(for: header, value: nil) {
             // we only use the index in this instance
             self.buffer.write(encodedInteger: UInt(index), prefix: 4, prefixBits: 0x10)
             // now append the value

Sources/NIOHPACK/HPACKErrors.swift

@@ -108,3 +108,11 @@ public enum NIOHPACKErrors {
         public init() { }
     }
 }
+
+/// This enum covers errors that are thrown internally for messaging reasons. These should
+/// not leak.
+internal enum InternalError: Error {
+    case unableToAddHeaderToTable(excess: Int)
+}
+
+extension InternalError: Hashable { }