avoid ByteString when parsing HTTP/2 headers#800
Draft
pjfanning wants to merge 4 commits intoapache:mainfrom
Draft
avoid ByteString when parsing HTTP/2 headers#800pjfanning wants to merge 4 commits intoapache:mainfrom
pjfanning wants to merge 4 commits intoapache:mainfrom
Conversation
Member
pjfanning
commented
Sep 26, 2025
pjfanning
commented
- experiment for Http2 header parser can be made more performant #798
- WIP
Member
Author
|
Breaks 1 Http2ClientSpec test |
pjfanning
force-pushed
the
http2-header
branch
from
5e2fe91 to
a1ac952
Compare
jrudolph
reviewed
Oct 2, 2025
| httpHeaderParser.parseHeaderLine(pekko.util.ByteString(concHeaderLine))() | ||
| httpHeaderParser.resultHeader | ||
| } else { | ||
| HttpHeader.parse(name, value, httpHeaderParser.settings) match { |
Contributor
There was a problem hiding this comment.
In general that's really cleaner. I don't expect a performance hit, there's a bit of a difference of how the parsers are looked up between HttpHeader.parse and httpHeaderParser.parseHeaderLine. HttpHeader.parse uses binary search to find the right parser and httpHeaderParser.parseHeaderLine uses kind of a lut / radix lookup once the parser is primed.
Member
Author
There was a problem hiding this comment.
Thanks @jrudolph.
I had to use the startsWith(":") check because HttpHeader.parse doesn't support pseudo headers and 1 test includes a :grpc-status pseudo header.
Contributor
There was a problem hiding this comment.
But wait, does it even parse it into anything useful?
Member
Author
There was a problem hiding this comment.
I'll debug it later. There is an existing test that relies on the header parser not failing. Whether it needs the parser to actually process the pseudo header at all is something that I haven't yet tested.
Contributor
There was a problem hiding this comment.
Maybe it's a typo in the test? Is :grpc-status a thing, i.e. is it allowed for gRPC (outside the HTTP/2 spec) to extend the set of : pseudo headers?
Contributor
There was a problem hiding this comment.
pekko-grpc also doesn't use the colon form so maybe it's just a typo and we could just fix the test.
Added comments to clarify handling of pseudo-headers in the parseHeaderPair method.
He-Pin
reviewed
Oct 2, 2025
| httpHeaderParser.parseHeaderLine(ByteString(concHeaderLine))() | ||
| httpHeaderParser.resultHeader | ||
| import HttpHeader.ParsingResult | ||
| if (name.startsWith(":")) { |
Member
There was a problem hiding this comment.
Maybe just a look ahead char is better
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.