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

Allow adding a sequence of headers to HTTPHeaders #1004

Merged
merged 4 commits into from May 16, 2019

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented May 16, 2019

Motivation:

When combining two collections of headers, adding each pair one-by-one
using add(name:value:) can cause multiple unnecessary reallocations of
the underlying array.

Modifications:

Provide two additional add methods to HTTPHeaders: one to add a sequence
of name/value pairs and another to add the headers from another
HTTPHeaders, both of which use append(contentsOf:) on the underlying
array to avoid additional reallocations.

Result:

Fewer reallocations when adding multiple headers.

Motivation:

When combining two collections of headers, adding each pair one-by-one
using add(name:value:) can cause multiple unnecessary reallocations of
the underlying array.

Modifications:

Provide two additional add methods to HTTPHeaders: one to add a sequence
of name/value pairs and another to add the headers from another
HTTPHeaders, both of which use `append(contentsOf:)` on the underlying
array to avoid additional reallocations.

Result:

Fewer reallocations when adding multiple headers.
@Lukasa Lukasa requested a review from weissi May 16, 2019 12:34
@Lukasa Lukasa added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label May 16, 2019
@Lukasa Lukasa added this to the 2.2.0 milestone May 16, 2019
Sources/NIOHTTP1/HTTPTypes.swift Show resolved Hide resolved
/// Add a sequence of header name/value pairs to the block.
///
/// This method is strictly additive: if there are other entries with the same header
/// name already in the block, this will add new entries. `add` performs case-insensitive
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment looks wrong: we don't compare the field names at all, except inasmuch as we're looking for the Connection header.

@inlinable
public mutating func add<S: Sequence>(contentsOf other: S) where S.Element == (String, String) {
precondition(!other.contains { !$0.0.utf8.contains(where: { !$0.isASCII }) }, "names must be ASCII")
self.headers.append(contentsOf: other)
Copy link
Member

@weissi weissi May 16, 2019

Choose a reason for hiding this comment

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

why doesn't this call self.add for every element (whilst reserving the capacity in self.headers beforehands)? Don't really like the logic duplication here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, makes sense!

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

thanks, that looks great

@weissi weissi requested a review from Lukasa May 16, 2019 15:07
@Lukasa Lukasa merged commit 6023c65 into apple:master May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants