add unit test for variable length encoding HTTP/3 #60766
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #51519
|
| @@ -149,7 +149,7 @@ public static bool TryWrite(Span<byte> buffer, long longToEncode, out int bytesW | |||
| Debug.Assert(longToEncode >= 0); | |||
| Debug.Assert(longToEncode <= EightByteLimit); | |||
|
|
|||
| if (longToEncode < OneByteLimit) | |||
| if (longToEncode <= OneByteLimit) | |||
There was a problem hiding this comment.
Is this a bug in header encoding? If so, what happens when an integer that is right at the boundary between limits triggers the bug?
There was a problem hiding this comment.
No, It's not bug, because we still produce correct values with more bytes (example 63 is encoded as 010000000 ,00111111 but can be juste 00111111). https://tools.ietf.org/html/draft-ietf-quic-transport-16#section-16 tells values at the edge can be encoded in less bytes so why using more.
There was a problem hiding this comment.
Ok, so those edge values are just not as efficient as they could be.
There was a problem hiding this comment.
Is that something that we should fix? If so, let's make sure we have an issue to track it, even if we think it's low-priority.
There was a problem hiding this comment.
It's not a bug, it's rather an optimization because we could encode for example 1 on 2 bytes or 4 bytes. Nothing prohibit that in the specs : Length ranges are cumulative
| 2Bit | Length | Usable Bits | Range |
|---|---|---|---|
| 00 | 1 | 6 | 0-63 |
| 01 | 2 | 14 | 0-16383 |
| 10 | 4 | 30 | 0-1073741823 |
| 11 | 8 | 62 | 0-4611686018427387903 |
There was a problem hiding this comment.
I understand it's not a bug; I'm asking if we can optimize this better than we do currently.
There was a problem hiding this comment.
There's similar edge case handling in loopback server:
https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/System/Net/Http/Http3LoopbackStream.cs#L171
Perhaps, it would be worth it to optimize the comparison there as well.
Otherwise looks good and thanks for the contribution.
|
Build Analysis failed with: This is also from other PRs. We should address it. @antonfirsov @wfurt can you please help here? |
ghost
locked as resolved and limited conversation to collaborators
Fixes #51519