Skip to content
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

Implementation of HPACK encoding and header tables #10

Merged
merged 30 commits into from
Aug 16, 2018
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
9b435f5
Implementation of HPACK routines, including integer compression, head…
Jun 27, 2018
dd514a4
Much updating and reworking based on pull request feedback:
Jul 10, 2018
88067f3
Updated based on PR feedback:
Jul 17, 2018
b304be4
Further changes based on PR feedback. RingBuffer now uses ByteBuffer …
Jul 17, 2018
7aaeb88
Removed code used to generate ring buffer view-copy metrics.
Jul 17, 2018
6a13d96
Updated Linux tests via script.
Jul 17, 2018
d5a36ec
Updated output of the HPACK integration tests to be a little less ver…
Jul 17, 2018
bbfa181
Write a description into our output HPACK test case stories.
Jul 17, 2018
678bbcc
Pruned some more SimpleRingBuffer methods that were no longer require…
Jul 17, 2018
e60f176
Fixed a couple of errors in the handling of wrapping writes in the ri…
Jul 18, 2018
c508938
Tweaked some documentation and formatting, and conceded defeat on the…
Jul 19, 2018
3541b8d
Numerous further changes based on PR feedback:
Jul 23, 2018
def5662
Tweaked some things in response to similar changes in `ByteBuffer`, a…
Jul 24, 2018
474a840
Some final (I think) adjustments to `StringRing` API.
Jul 24, 2018
ef5f776
HPACK encoder now has a more graceful (and correct!) way of handling …
Jul 25, 2018
a58a1b0
Some industrious use of `@_specialized` and `@_inlineable` on generic…
Jul 26, 2018
3f0fdda
Updated documentation comments on dynamic header table sizes.
AlanQuatermain Jul 26, 2018
082a5b3
Tweaked return types of `HeaderTable.findHeaders(matching:)` and `Hea…
AlanQuatermain Jul 27, 2018
41602c1
HeaderTables no longer type-erase the lazy collections returned from …
AlanQuatermain Jul 30, 2018
02a3c00
Updated swift-nio dependency to 1.9.0.
AlanQuatermain Aug 1, 2018
93275b0
Updated to use new `ByteBuffer.reserveCapacity()` API.
AlanQuatermain Aug 1, 2018
f811f14
Updated Linux tests via script.
AlanQuatermain Aug 2, 2018
6afb9be
Added missing license headers and updated CONTRIBUTORS.txt via script.
AlanQuatermain Aug 2, 2018
8e054c6
Integration tests now use Foundation types for timers instead of CF, …
AlanQuatermain Aug 3, 2018
7e11dd0
Recreated hpack-test-status submodule due to a weird glitch that caus…
AlanQuatermain Aug 3, 2018
0080714
Unit tests now build and run on Linux as expected.
AlanQuatermain Aug 3, 2018
c8e5891
Changes based on review from @weissi
AlanQuatermain Aug 7, 2018
43d8ad5
Updated implementation based on PR feedback. Removed attributes where…
AlanQuatermain Aug 8, 2018
7e5a322
Updated email for @tomerd in contributors list and mailmap.
AlanQuatermain Aug 14, 2018
15e9de9
Merge branch 'master' into master
Lukasa Aug 16, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[submodule "hpack-test-case"]
path = hpack-test-case
url = [email protected]:http2jp/hpack-test-case
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick question for @normanmaurer . Do we need to add this to NOTICE.txt? It's only used for tests.

1 change: 1 addition & 0 deletions .mailmap
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
Johannes Weiß <[email protected]>
<[email protected]> <[email protected]>
<[email protected]> <[email protected]>
<[email protected]> <[email protected]>
4 changes: 4 additions & 0 deletions CONTRIBUTORS.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ needs to be listed here.
### Contributors

- Cory Benfield <[email protected]>
- Jim Dovey <[email protected]>
- Johannes Weiß <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mind making this read

Johannes Weiss <[email protected]>

just so my work email address is listed there, sorry

- Johannes Weiß <[email protected]>
- Tim <[email protected]>
- tomer doron <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use my apple email address? [email protected]


**Updating this list**

Expand Down
8 changes: 6 additions & 2 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ let package = Package(
name: "swift-nio-http2",
products: [
.executable(name: "NIOHTTP2Server", targets: ["NIOHTTP2Server"]),
.library(name: "NIOHTTP2", targets: ["NIOHTTP2"])
.library(name: "NIOHTTP2", targets: ["NIOHTTP2"]),
],
dependencies: [
.package(url: "https://github.com/apple/swift-nio.git", from: "1.7.0"),
.package(url: "https://github.com/apple/swift-nio.git", from: "1.9.0"),
.package(url: "https://github.com/apple/swift-nio-nghttp2-support.git", from: "1.0.0"),
],
targets: [
Expand All @@ -31,7 +31,11 @@ let package = Package(
dependencies: ["NIOHTTP2"]),
.target(name: "NIOHTTP2",
dependencies: ["NIO", "NIOHTTP1", "NIOTLS", "CNIONghttp2"]),
.target(name: "NIOHPACK",
dependencies: ["NIO", "NIOConcurrencyHelpers", "NIOHTTP1"]),
.testTarget(name: "NIOHTTP2Tests",
dependencies: ["NIO", "NIOHTTP1", "NIOHTTP2"]),
.testTarget(name: "NIOHPACKTests",
dependencies: ["NIOHPACK"])
]
)
177 changes: 177 additions & 0 deletions Sources/NIOHPACK/DynamicHeaderTable.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the SwiftNIO open source project
//
// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors
// Licensed under Apache License v2.0
//
// See LICENSE.txt for license information
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
//
// SPDX-License-Identifier: Apache-2.0
//
//===----------------------------------------------------------------------===//

import NIO

/// Implements the dynamic part of the HPACK header table, as defined in
/// [RFC 7541 § 2.3](https://httpwg.org/specs/rfc7541.html#dynamic.table).
struct DynamicHeaderTable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This structure should get a doc comment.

public static let defaultSize = 4096

/// The actual table, with items looked up by index.
private var storage: HeaderTableStorage

/// The length of the contents of the table.
var length: Int {
return self.storage.length
}

/// The size to which the dynamic table may currently grow. Represents
/// the current maximum length signaled by the peer via a table-resize
/// value at the start of an encoded header block.
///
/// - note: This value cannot exceed `self.maximumTableLength`.
var allowedLength: Int {
get {
return self.storage.maxSize
}
set {
self.storage.setTableSize(to: newValue)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than lazily calculate this I'd be inclined to incur this cost eagerly, as it's relatively minor and greatly reduces the cost of the more common header operations.


/// The maximum permitted size of the dynamic header table as set
/// through a SETTINGS_HEADER_TABLE_SIZE value in a SETTINGS frame.
var maximumTableLength: Int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this and allowedLength need more documentation to distinguish them from each other.

didSet {
if self.allowedLength > maximumTableLength {
self.allowedLength = maximumTableLength
}
}
}

/// The number of items in the table.
var count: Int {
return self.storage.count
}

init(allocator: ByteBufferAllocator, maximumLength: Int = DynamicHeaderTable.defaultSize) {
self.storage = HeaderTableStorage(allocator: allocator, maxSize: maximumLength)
self.maximumTableLength = maximumLength
self.allowedLength = maximumLength // until we're told otherwise, this is what we assume the other side expects.
}

/// Subscripts into the dynamic table alone, using a zero-based index.
subscript(i: Int) -> HeaderTableEntry {
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)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we document that our dynamic table is zero indexed for the purposes of this subscript?


// internal for testing -- clears the dynamic table
mutating func clear() {
self.storage.purge(toRelease: self.storage.length)
}

/// Searches the table for a matching header, optionally with a particular value. If
/// a match is found, returns the index of the item and an indication whether it contained
/// the matching value as well.
///
/// Invariants: If `value` is `nil`, result `containsValue` is `false`.
///
/// - Parameters:
/// - name: The name of the header for which to search.
/// - value: Optional value for the header to find. Default is `nil`.
/// - 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.
@_specialize(where Name == String.UTF8View, Value == String.UTF8View) // from String-based API
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we actually need to manually specialise here? We use them both from within this module so it should already be specialised, no? I don't think it's decided yet that @_specialize will become public as @specialize so if possible I'd prefer not to use it at least until it is. @Lukasa what's your take?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested it mainly to defend against running into problems. I'm not wedded to it, but it strikes me that we're unlikely to see a meaningful change to _specialize.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lukasa well, syntax might be different. Or the whole same set of problems that we had with @_inlineable and @_versioned: Fortunately Slava was nice enough to take the warnings out of the language version 4 but they could've also just renamed the attribute without a warning...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove them for now, and they can be re-added if necessary in a performance-focussed update. They caused a crash in the Swift 4.2 compiler anyway…

Personally I prefer getting the functionality in place with one commit, then adding the more esoteric performance adjustments separately. First commit works, more can be added for optimization and fine-tuning, of which there may be some here as the HTTP/2 framer implementation evolves.

@_specialize(where Name == ByteBufferView, Value == ByteBufferView) // from HPACKHeaders-based API
func findExistingHeader<Name: Collection, Value: Collection>(named name: Name, value: Value?) -> (index: Int, containsValue: Bool)? where Name.Element == UInt8, Value.Element == UInt8 {
// 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 {
// no `first` on AnySequence, just `first(where:)`
return self.storage.firstIndex(matching: name).map { ($0, false) }
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findExistingHeader currently does a linear scan of the whole table even when it is only looking for the first instance of a name, disregarding the value. That's not necessary: we can exit early. We should either make indices produce a lazy Collection, or alternatively have a firstIndex(matching:) method that can exit early.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further discussion I think a lazy Collection or Sequence is the way to go here, even for the case when we scan the whole table. We don't need an array allocation here really, and if we can take that allocation off the hot path it'd be very helpful.

This means HeaderTableStorage will need an as-private-as-possible method func index<Name: Collection>(matching: Name, after index: Int) -> Int? to base that iterator off, but that's fine, I think. (Incidentally, the initial value to after should be -1 ;) ).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the standard library can help us here:

return self.headers.enumerated().lazy.filter {
    self.buffer.equalCaseInsensitiveASCII(view: name, at: $1.name)
}.map { (idx, header) in idx }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's for indices(matching:). findHeaders(matching:) is a little nicer, since it skips the enumerated() and thus the map(_:):

return self.headers.lazy.filter { self.buffer.equalCaseInsensitiveASCII(view: name, at: $0.name) }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I've started it, but the Swift 4.2 compiler segfaults when building the unit tests. Sigh. 4.1 seems ok though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holy crap the new compiler optimizes well. Unit tests with a release build went from running in 14 seconds to 32 when switching to the current release version of Xcode 😮

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this is all done now, and everything seems happy in shipping Xcode etc. I'll give it another run with the beta toolset and see if I can't get a decent bug report.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, still segfaults while compiling an @_specialized method used by the unit tests. Only when building the unit tests, though.

1.	While running pass #55813 SILFunctionTransform "SimplifyCFG" on SILFunction "@$S8NIOHPACK18IndexedHeaderTableV3add15headerNameBytes05valueH0yx_q_tK3NIO20ContiguousCollectionRzAgHR_s5UInt8V7ElementRtzAjKRt_r0_lF".
 for 'add(headerNameBytes:valueBytes:)' at /Users/jdovey/Projects/Github/Quatermain/swift-nio-http2/Sources/NIOHPACK/IndexedHeaderTable.swift:146:21
error: Segmentation fault: 11

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed a bug with the Swift team: SR-8386.


// If we have a value, locate the index of the lowest header which contains that
// value, but if no value matches, return the index of the lowest header with a
// matching name alone.
var firstNameMatch: Int? = nil
for index in self.storage.indices(matching: name) {
if firstNameMatch == nil {
// record the first (most recent) index with a matching header name,
// in case there's no value match.
firstNameMatch = index
}

if self.storage.view(of: self.storage[index].value).matches(value) {
// this entry has both the name and the value we're seeking
return (index, true)
}
}

// no value matches -- but did we find a name?
if let index = firstNameMatch {
return (index, false)
} else {
// no matches at all
return nil
}
}

/// Appends a header to the table. Note that if this succeeds, the new item's index
/// is always zero.
///
/// This call may result in an empty table, as per RFC 7541 § 4.4:
/// > "It is not an error to attempt to add an entry that is larger than the maximum size;
/// > an attempt to add an entry larger than the maximum size causes the table to be
/// > 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.
/// - Returns: `true` if the header was added to the table, `false` if not.
@_specialize(where Name == String.UTF8View, Value == String.UTF8View) // from String-based API
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment here

mutating func addHeader<Name: Collection, Value: Collection>(named name: Name, value: Value) throws where Name.Element == UInt8, Value.Element == UInt8 {
do {
try self.storage.add(name: name, value: value)
} catch let error as RingBufferError.BufferOverrun {
// ping the error up the stack, with more information
throw NIOHPACKErrors.FailedToAddIndexedHeader(bytesNeeded: self.storage.length + error.amount,
name: name, value: value)
}
}

/// Appends a header to the table. Note that if this succeeds, the new item's index
/// is always zero.
///
/// This call may result in an empty table, as per RFC 7541 § 4.4:
/// > "It is not an error to attempt to add an entry that is larger than the maximum size;
/// > an attempt to add an entry larger than the maximum size causes the table to be
/// > emptied of all existing entries and results in an empty table."
///
/// - Parameters:
/// - name: A contiguous collection of UTF-8 bytes comprising the name of the header to insert.
/// - value: A contiguous collection of UTF-8 bytes comprising the value of the header to insert.
/// - Returns: `true` if the header was added to the table, `false` if not.
@_specialize(where Name == ByteBufferView, Value == ByteBufferView) // from HPACKHeaders-based API
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

mutating func addHeader<Name: ContiguousCollection, Value: ContiguousCollection>(nameBytes: Name, valueBytes: Value) throws where Name.Element == UInt8, Value.Element == UInt8 {
do {
try self.storage.add(nameBytes: nameBytes, valueBytes: valueBytes)
} catch let error as RingBufferError.BufferOverrun {
// convert the error to something more useful/meaningful to client code.
throw NIOHPACKErrors.FailedToAddIndexedHeader(bytesNeeded: self.storage.length + error.amount,
name: nameBytes, value: valueBytes)
}
}
}
Loading