Make ACK_RECEIVE_TIMESTAMPS frame Gap more efficient and QUIC-like. #6
Conversation
This file contains 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
Per TBD IETF receive timestamp proposal: https://github.com/wcsmith/draft-quic-receive-ts Gap: A variable-length integer indicating the largest packet number in the Timestamp Range as follows: For the first Timestamp Range: Gap is the difference between (a) the Largest Acknowledged packet number in the frame and (b) the largest packet in the current (first) Timestamp Range. For subsequent Timestamp Ranges: Gap is the difference between (a) the packet number two lower than the smallest packet number in the previous Timestamp Range and (b) the largest packet in the current Timestamp Range.
ianswett
reviewed
Oct 24, 2021
wcsmith
force-pushed
the
receive-timestamps-gap
branch
from
bc52a60
to
1050cd1
Compare
|
I have a couple more pending changes, so I also started #8 where I'll keep adding additional commits (didn't want to change/remove this one out from under you, but feel free to close this one and dedup there). Clearly I'm still learning how to do these public git review workflows :). |
|
Sounds good, I merged it and we can follow up on anything else in #8 |
copybara-service bot
pushed a commit
that referenced
this pull request
Feb 2, 2022
*** Reason for rollback *** This causes test failures in Chromium. I would normally fix it, but currently QUICHE roll to Chromium is blocked on another CL and there are two other CLs that are complicated to roll. Please allow me to roll this one back for now, I'll be happy to help with debugging after I am able to roll the latest QUICHE into Chromium. Error is: [ RUN ] HeaderValidatorTest.NameHasInvalidChar ../../buildtools/third_party/libc++/trunk/include/array:205: _LIBCPP_ASSERT '__n < _Size' failed. out-of-bounds access in std::array<T, N> Received signal 6 #0 0x7fe693f3799f base::debug::CollectStackTrace() #1 0x7fe693c8fd3a base::debug::StackTrace::StackTrace() #2 0x7fe693c8fcf5 base::debug::StackTrace::StackTrace() #3 0x7fe693f3746c base::debug::(anonymous namespace)::StackDumpSignalHandler() #4 0x7fe6911f1200 (/lib/x86_64-linux-gnu/libpthread-2.33.so+0x131ff) #5 0x7fe690db3891 gsignal #6 0x7fe690d9d536 abort #7 0x7fe6912c941c std::__Cr::__libcpp_abort_debug_function() #8 0x7fe6953c9996 std::__Cr::array<>::operator[]() #9 0x7fe6953c95c9 http2::adapter::(anonymous namespace)::AllCharsInMap() #10 0x7fe6953c87ec http2::adapter::(anonymous namespace)::IsValidHeaderName() #11 0x7fe6953c7e15 http2::adapter::HeaderValidator::ValidateSingleHeader() #12 0x56309a95f81d http2::adapter::test::HeaderValidatorTest_NameHasInvalidChar_Test::TestBody() #13 0x56309bf0b83b testing::internal::HandleSehExceptionsInMethodIfSupported<>() #14 0x56309befc767 testing::internal::HandleExceptionsInMethodIfSupported<>() #15 0x56309beeaac1 testing::Test::Run() #16 0x56309beeb1d7 testing::TestInfo::Run() #17 0x56309beeb83d testing::TestSuite::Run() #18 0x56309bef5e7a testing::internal::UnitTestImpl::RunAllTests() #19 0x56309bf0fa3b testing::internal::HandleSehExceptionsInMethodIfSupported<>() #20 0x56309befe337 testing::internal::HandleExceptionsInMethodIfSupported<>() #21 0x56309bef5a11 testing::UnitTest::Run() #22 0x56309c167aa1 RUN_ALL_TESTS() *** Original change description *** Use static char maps in HeaderValidator::ValidateSingleHeader(). When validating header names/values against allowed characters, switching to static char maps may help with performance. This CL is otherwise not a functional change. *** PiperOrigin-RevId: 425943183
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Per TBD IETF receive timestamp proposal:
https://github.com/wcsmith/draft-quic-receive-ts
Gap: A variable-length integer indicating the largest packet number
in the Timestamp Range as follows: