Update shared HPackEncoder #32944
Update shared HPackEncoder #32944
Conversation
|
|
||
| public bool BeginEncode(IEnumerable<KeyValuePair<string, string>> headers, Span<byte> buffer, out int length) | ||
| public bool BeginEncode(HeadersEnumerator enumerator, Span<byte> buffer, out int length) |
There was a problem hiding this comment.
I thought the point was that HttpClient didn't need these APIs and they could be removed?
There was a problem hiding this comment.
HttpClient might not use this code but non-Kestrel code in our repo does, e.g. HttpSys tests, httpcat.
I changed my PR to use an #if. It minimizes the amount of overall change.
There was a problem hiding this comment.
Minimizing changes is not critical at this point, I'd rather have a clean result. If only test code needs these old APIs then is there something straightforward we can do to restructure how they call this?
In the design for dynamic header compression we've already proposed changes that would require moving this enumerator outside of HPackEncoder. Maybe now is a good time to separate them.
There was a problem hiding this comment.
If only test code needs these old APIs
Kestrel and tests use these APIs. My previous copied and pasted class that wrapped HPackEncoder still was the same API. I have just changed separation of our custom enumerator to use an #if.
src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackEncoder.cs
Show resolved
Hide resolved
|
I don't have write permissions. Someone who does will need to merge it. |
There was a problem hiding this comment.
It looks fine, but I think it'd be worth looking at why the encoder needs to hold a copy of the enumerator -- from high-level view it seems like this API can go away, the field can go away, and the class made static (until we add dynamic table encoding, then it'll need some other state).
|
/azp list |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Agreed. That's being investigated in the next PR. dotnet/aspnetcore#19521 |
ghost
locked as resolved and limited conversation to collaborators
Sync changes from dotnet/aspnetcore#19393. Review now, but wait until that PR is merged before merging this.
Optimizing the enumerator eliminates allocations per HTTP request.