Skip to content

PseudoHeaderFields adopting CoW to reduce size #88

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

Merged
merged 4 commits into from
Feb 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
113 changes: 108 additions & 5 deletions Sources/HTTPTypes/HTTPRequest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -146,53 +146,156 @@ public struct HTTPRequest: Sendable, Hashable {

/// The pseudo header fields of a request.
public struct PseudoHeaderFields: Sendable, Hashable {
private final class _Storage: @unchecked Sendable, Hashable {
var method: HTTPField
var scheme: HTTPField?
var authority: HTTPField?
var path: HTTPField?
var extendedConnectProtocol: HTTPField?

init(
method: HTTPField,
scheme: HTTPField?,
authority: HTTPField?,
path: HTTPField?,
extendedConnectProtocol: HTTPField?
) {
self.method = method
self.scheme = scheme
self.authority = authority
self.path = path
self.extendedConnectProtocol = extendedConnectProtocol
}

func copy() -> Self {
.init(
method: self.method,
scheme: self.scheme,
authority: self.authority,
path: self.path,
extendedConnectProtocol: self.extendedConnectProtocol
)
}

static func == (lhs: _Storage, rhs: _Storage) -> Bool {
lhs.method == rhs.method && lhs.scheme == rhs.scheme && lhs.authority == rhs.authority
&& lhs.path == rhs.path && lhs.extendedConnectProtocol == rhs.extendedConnectProtocol
}

func hash(into hasher: inout Hasher) {
hasher.combine(self.method)
hasher.combine(self.scheme)
hasher.combine(self.authority)
hasher.combine(self.path)
hasher.combine(self.extendedConnectProtocol)
}
}

private var _storage: _Storage

/// The underlying ":method" pseudo header field.
///
/// The value of this field must be a valid method.
///
/// https://www.rfc-editor.org/rfc/rfc9110.html#name-methods
public var method: HTTPField {
willSet {
get {
self._storage.method
}
set {
precondition(newValue.name == .method, "Cannot change pseudo-header field name")
precondition(HTTPField.isValidToken(newValue.rawValue._storage), "Invalid character in method field")

if !isKnownUniquelyReferenced(&self._storage) {
self._storage = self._storage.copy()
}
self._storage.method = newValue
}
}

/// The underlying ":scheme" pseudo header field.
public var scheme: HTTPField? {
willSet {
get {
self._storage.scheme
}
set {
if let name = newValue?.name {
precondition(name == .scheme, "Cannot change pseudo-header field name")
}

if !isKnownUniquelyReferenced(&self._storage) {
self._storage = self._storage.copy()
}
self._storage.scheme = newValue
}
}

/// The underlying ":authority" pseudo header field.
public var authority: HTTPField? {
willSet {
get {
self._storage.authority
}
set {
if let name = newValue?.name {
precondition(name == .authority, "Cannot change pseudo-header field name")
}

if !isKnownUniquelyReferenced(&self._storage) {
self._storage = self._storage.copy()
}
self._storage.authority = newValue
}
}

/// The underlying ":path" pseudo header field.
public var path: HTTPField? {
willSet {
get {
self._storage.path
}
set {
if let name = newValue?.name {
precondition(name == .path, "Cannot change pseudo-header field name")
}

if !isKnownUniquelyReferenced(&self._storage) {
self._storage = self._storage.copy()
}
self._storage.path = newValue
}
}

/// The underlying ":protocol" pseudo header field.
public var extendedConnectProtocol: HTTPField? {
willSet {
get {
self._storage.extendedConnectProtocol
}
set {
if let name = newValue?.name {
precondition(name == .protocol, "Cannot change pseudo-header field name")
}

if !isKnownUniquelyReferenced(&self._storage) {
self._storage = self._storage.copy()
}
self._storage.extendedConnectProtocol = newValue
Copy link

Choose a reason for hiding this comment

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

A bit of a nit, but these accessors probably want to either replace set with _modify or to use @inlinable. Otherwise minor modifications can risk CoWing the underlying strings where it isn't necessary to do so.

This isn't a huge deal as I don't think there's a lot of minor string modifications happening this way (pretty rare to do request.path.value += "/something/else") but it'd be nice to avoid the CoW in that case.

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'm not very familiar with _modify. Does this look reasonable?

            _modify {
                if !isKnownUniquelyReferenced(&self._storage) {
                    self._storage = self._storage.copy()
                }
                yield &self._storage.path
                if let name = self._storage.path?.name {
                    precondition(name == .path, "Cannot change pseudo-header field name")
                }
            }

Copy link

Choose a reason for hiding this comment

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

I believe that will be fine, though _modify is extremely sensitive to the exact nature of its flow control. I recommend actually building this under -O and checking that there isn't a call to malloc or similar there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a call to malloc after this change. Not sure how to get rid of it.

libHTTPTypes.dylib.zip

Screenshot 2025-02-17 at 3 40 44 PM

Copy link

Choose a reason for hiding this comment

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

Will take a look

}
}

init(
method: HTTPField,
scheme: HTTPField?,
authority: HTTPField?,
path: HTTPField?,
extendedConnectProtocol: HTTPField? = nil
) {
self._storage = .init(
method: method,
scheme: scheme,
authority: authority,
path: path,
extendedConnectProtocol: extendedConnectProtocol
)
}
}

/// The pseudo header fields.
Expand Down
83 changes: 64 additions & 19 deletions Sources/HTTPTypes/HTTPResponse.swift
Original file line number Diff line number Diff line change
Expand Up @@ -162,32 +162,88 @@ public struct HTTPResponse: Sendable, Hashable {
let code =
Int(codeIterator.next()! - 48) * 100 + Int(codeIterator.next()! - 48) * 10
+ Int(codeIterator.next()! - 48)
return Status(uncheckedCode: code, reasonPhrase: self.reasonPhrase)
return Status(uncheckedCode: code, reasonPhrase: self.pseudoHeaderFields.reasonPhrase)
}
set {
self.pseudoHeaderFields.status.rawValue = ISOLatin1String(unchecked: newValue.fieldValue)
self.reasonPhrase = newValue.reasonPhrase
self.pseudoHeaderFields.reasonPhrase = newValue.reasonPhrase
}
}

/// The pseudo header fields of a response.
public struct PseudoHeaderFields: Sendable, Hashable {
private final class _Storage: @unchecked Sendable, Hashable {
var status: HTTPField
var reasonPhrase: String

init(status: HTTPField, reasonPhrase: String) {
self.status = status
self.reasonPhrase = reasonPhrase
}

func copy() -> Self {
.init(
status: self.status,
reasonPhrase: self.reasonPhrase
)
}

static func == (lhs: _Storage, rhs: _Storage) -> Bool {
lhs.status == rhs.status
}

func hash(into hasher: inout Hasher) {
hasher.combine(self.status)
}
}

private var _storage: _Storage

/// The underlying ":status" pseudo header field.
///
/// The value of this field must be 3 ASCII decimal digits.
public var status: HTTPField {
willSet {
get {
self._storage.status
}
set {
Copy link

Choose a reason for hiding this comment

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

Same note on these accessors re _modify.

precondition(newValue.name == .status, "Cannot change pseudo-header field name")
precondition(Status.isValidStatus(newValue.rawValue._storage), "Invalid status code")

if !isKnownUniquelyReferenced(&self._storage) {
self._storage = self._storage.copy()
}
self._storage.status = newValue
}
}

var reasonPhrase: String {
get {
self._storage.reasonPhrase
}
set {
if !isKnownUniquelyReferenced(&self._storage) {
self._storage = self._storage.copy()
}
self._storage.reasonPhrase = newValue
}
}

private init(status: HTTPField) {
self._storage = .init(status: status, reasonPhrase: "")
}

init(status: Status) {
self._storage = .init(
status: HTTPField(name: .status, uncheckedValue: ISOLatin1String(unchecked: status.fieldValue)),
reasonPhrase: status.reasonPhrase
)
}
}

/// The pseudo header fields.
public var pseudoHeaderFields: PseudoHeaderFields

private var reasonPhrase: String

/// The response header fields.
public var headerFields: HTTPFields

Expand All @@ -196,20 +252,9 @@ public struct HTTPResponse: Sendable, Hashable {
/// - status: The status code and an optional reason phrase.
/// - headerFields: The response header fields.
public init(status: Status, headerFields: HTTPFields = [:]) {
let statusField = HTTPField(name: .status, uncheckedValue: ISOLatin1String(unchecked: status.fieldValue))
self.pseudoHeaderFields = .init(status: statusField)
self.reasonPhrase = status.reasonPhrase
self.pseudoHeaderFields = .init(status: status)
self.headerFields = headerFields
}

public func hash(into hasher: inout Hasher) {
hasher.combine(self.pseudoHeaderFields)
hasher.combine(self.headerFields)
}

public static func == (lhs: HTTPResponse, rhs: HTTPResponse) -> Bool {
lhs.pseudoHeaderFields == rhs.pseudoHeaderFields && lhs.headerFields == rhs.headerFields
}
}

extension HTTPResponse: CustomDebugStringConvertible {
Expand Down Expand Up @@ -273,7 +318,7 @@ extension HTTPResponse: Codable {
public func encode(to encoder: Encoder) throws {
var container = encoder.container(keyedBy: CodingKeys.self)
try container.encode(self.pseudoHeaderFields, forKey: .pseudoHeaderFields)
try container.encode(self.reasonPhrase, forKey: .reasonPhrase)
try container.encode(self.pseudoHeaderFields.reasonPhrase, forKey: .reasonPhrase)
try container.encode(self.headerFields, forKey: .headerFields)
}

Expand All @@ -288,7 +333,7 @@ extension HTTPResponse: Codable {
guard Status.isValidReasonPhrase(reasonPhrase) else {
throw DecodingError.invalidReasonPhrase(reasonPhrase)
}
self.reasonPhrase = reasonPhrase
self.pseudoHeaderFields.reasonPhrase = reasonPhrase
self.headerFields = try container.decode(HTTPFields.self, forKey: .headerFields)
}
}
Expand Down
6 changes: 6 additions & 0 deletions Tests/HTTPTypesTests/HTTPTypesTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -248,4 +248,10 @@ final class HTTPTypesTests: XCTestCase {
XCTAssertEqual(trailerFields[HTTPField.Name("trailer1")!], "value1")
XCTAssertEqual(trailerFields[HTTPField.Name("trailer2")!], "value2")
}

func testTypeLayoutSize() {
XCTAssertEqual(MemoryLayout<HTTPRequest>.size, MemoryLayout<AnyObject>.size * 2)
XCTAssertEqual(MemoryLayout<HTTPResponse>.size, MemoryLayout<AnyObject>.size * 2)
XCTAssertEqual(MemoryLayout<HTTPFields>.size, MemoryLayout<AnyObject>.size)
}
}
Loading