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

Reduce allocations when peeking pseudo-headers. #246

Merged
merged 1 commit into from
Sep 24, 2020

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Sep 23, 2020

Motivation:

The 2 to 1 codecs do a number of operations where they "peek" the
pseudo-headers. Each of these operations uses the HPACKHeaders
subscript, meaning that they allocate a temporary array for a field that
can, in a correct request/response, contain only one element. That's
very wasteful!

Modifications:

  • Rewrite peekPseudoHeader to iterate instead, avoiding the array.
  • Add allocaton counter test.

Result:

4 fewer allocations per HTTP request transformed on the server side,
2 fewer allocations per HTTP response transformed on the client side.
Resolves #244.

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Sep 23, 2020
@Lukasa Lukasa requested a review from glbrntt September 23, 2020 16:42
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Nice one! Just one small clarifying question.

var headerValue: String? = nil

for (fieldName, fieldValue, _) in self {
if name == fieldName {
Copy link
Contributor

Choose a reason for hiding this comment

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

The subscript does a case insensitive check, I assume we don't need to do that here because this is only ever called internally and we know that the headers must be lowercase so we'll always provide a lowercased 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.

Pseudo-headers are required to be lower-case. This is a nice perf win here: if the weren't lowercase, the request was malformed anyway.

Motivation:

The 2 to 1 codecs do a number of operations where they "peek" the
pseudo-headers. Each of these operations uses the HPACKHeaders
subscript, meaning that they allocate a temporary array for a field that
can, in a correct request/response, contain only one element. That's
very wasteful!

Modifications:

- Rewrite peekPseudoHeader to iterate instead, avoiding the array.
- Add allocaton counter test.

Result:

4 fewer allocations per HTTP request transformed on the server side,
2 fewer allocations per HTTP response transformed on the client side.
@Lukasa Lukasa force-pushed the cb-better-header-iteration branch from 1993e81 to 8ae9214 Compare September 23, 2020 17:21
@glbrntt glbrntt merged commit 5b0194a into apple:master Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HPACKHeaders.peekPseudoHeader allocates awkward temporary array
3 participants