Skip to content

Eliminate HTTP2 HPack enumerator allocations #19393

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 3 commits into from
Mar 2, 2020

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Feb 27, 2020

Fixes #19275

Creates a shared struct "enumerator" that wraps the two possible struct enumerator types (avoids boxing to IEnumerator) and handles enumerating multiple strings in StringValue (avoids allocation from yield).

Long term we should improve the way ASP.NET Core does HPack and try to unify more with the client, but in the meantime this change eliminates 4 allocations per HTTP/2 request.

There are some minor changes to shared HTTP2 code that will need to be mirrored.

@JamesNK JamesNK requested a review from a team February 27, 2020 10:16
@JamesNK
Copy link
Member Author

JamesNK commented Feb 27, 2020

Before:

image

After:

image

@JamesNK
Copy link
Member Author

JamesNK commented Feb 27, 2020

I've changed this PR to use an #if, and our code continues to use HPackEncoder directly. Reasons:

  • It minimizes the amount of overall change.
  • HttpClient might not use HPackEncoder.BeginEncode but non-Kestrel code in our repo does, e.g. HttpSys tests, httpcat.

@JamesNK JamesNK force-pushed the jamesnk/http2-hpack-enumerator branch from 3be08ce to ac08759 Compare February 28, 2020 01:01
@JamesNK
Copy link
Member Author

JamesNK commented Feb 29, 2020

Yield (current)

               Method |     Mean |    Error |   StdDev |        Op/s |  Gen 0 | Allocated |
--------------------- |---------:|---------:|---------:|------------:|-------:|----------:|
 WriteResponseHeaders | 604.2 ns | 3.755 ns | 3.512 ns | 1,655,046.7 | 0.0029 |     232 B |

Struct enumerator

               Method |     Mean |    Error |   StdDev |        Op/s | Allocated |
--------------------- |---------:|---------:|---------:|------------:|----------:|
 WriteResponseHeaders | 664.2 ns | 6.750 ns | 4.881 ns | 1,505,680.8 |      32 B |

Class enumerator as IEnumerator

               Method |     Mean |    Error |   StdDev |        Op/s | Allocated |
--------------------- |---------:|---------:|---------:|------------:|----------:|
 WriteResponseHeaders | 568.8 ns | 3.868 ns | 3.618 ns | 1,758,145.7 |      32 B |

Class enumerator

               Method |     Mean |    Error |   StdDev |        Op/s | Allocated |
--------------------- |---------:|---------:|---------:|------------:|----------:|
 WriteResponseHeaders | 526.4 ns | 5.163 ns | 4.829 ns | 1,899,719.1 |      32 B |

A cached enumerator on the frame writer and HPackEncoder taking that type directly (avoid interface calls on IEnumerator) is fastest.

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

LGTM

@JamesNK JamesNK merged commit f34e812 into master Mar 2, 2020
@JamesNK JamesNK deleted the jamesnk/http2-hpack-enumerator branch March 2, 2020 23:37
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP2: Header enumerator allocations
7 participants