Support ACK_RECIEVE_TIMESTAMPS extension. #8
Conversation
…ake. PiperOrigin-RevId: 404889656
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.
Make ACK_RECEIVE_TIMESTAMPS frame Gap more efficient and QUIC-like.
wcsmith
force-pushed
the
receive-timestamps
branch
from
1b094ad
to
7b437b0
Compare
Per TBD IETF receive timestamp proposal: https://github.com/wcsmith/draft-quic-receive-ts
wcsmith
force-pushed
the
receive-timestamps
branch
from
7b437b0
to
06fffc1
Compare
|
Note PR commits have been re-pushed. |
|
Thanks for the update, that PR just got a lot larger! |
| // ECN counts. | ||
| if (frame.ecn_counters_populated && | ||
| if (process_timestamps_) { | ||
| ack_frame_size += GetIetfAckFrameTimestampSize(frame); |
There was a problem hiding this comment.
I believe the existing code has logic to truncate the timestamps if they don't fit, so this size is a bit 'flexible' and not a minimum. If you changed the method name to GetMinAckFrameSize(Similar to GetMinCryptFrameSize and GetMinStreamFrameSize) that included no or 1 timestamp at a minimum, then it'd be quite a bit simpler. In practice, the timestamp frame would basically never get truncated unless you had so many timestamps they didn't fit into a packet.
I think this would allow GetIetfAckFrameTimestampSize to be much simpler? As it is, it's almost identical to the serialization code.
There was a problem hiding this comment.
Makes sense to allow truncating the ACK to omit timestamps if they won't fit (a nice future improvement might be to send as many as there are room for).
Is there a way to roll back the data writer to a previous position? Or does this make sense to add? This was what I wanted to do originally (save the buffer position before writing timestamps, and then roll it back if writing timestamps failed), but I didn't see any way to do this with the data writer--I could add though if it makes sense.
Without this though, I think a size method may still be needed because for truncation you'd still need to check "will the timestamps fit" before you write them (otherwise any time you chose to truncate, if the timestamps didn't fit, the whole packet build would fail).
|
Just a reminder that because of technical limitations, external PR cannot be merged to GitHub directly. Somebody will manually clone and land the PR upstream instead (after the PR is approved here), which will be mirrored here by some automated tools. After approval, this PR will need to be closed without merging. |
Ack, thanks--this is the model Ian and I discussed as well. |
Hopefully we can get rid of the duplicated logic for size, at which point hopefully it will mostly be tests :) |
|
As we discussed, it seems easiest to change he method to GetMinIetfAckFrameTimestampSize and return 1 byte or another small value that is easy to calculate and will always fit into a single QUIC packet. You'll have to change the writing code to exit early if it runs out of room, which I think is what the other timestamp writing code already does. Thanks, ian |
|
Hi Connor, did you want to keep working on this or hand it off to someone internally at Google? |
*** 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
|
@ianswett my apologies for the radio silence. Back before the break, I was intending to discuss further whether you wanted to keep pursuing this path given the IETF feeback of decoupling feedback from the ack frame. In the meantime it looks like @wu-bin has made some great progress on this (thank you!). Is there anything left for this version of implementation at this point? Or is the implementation as written in our draft done? |
|
Thanks for following up. I think the implementation now matches the draft and we can close this PR. At this point, I might be inclined to get some implementation experience and see if anyone in the Media Over QUIC group has interest in an extension like this? |
|
Cool, sounds good to me! I'll follow up with you in a separate channel about the moq socialization. |
Per TBD IETF receive timestamp proposal:
https://github.com/wcsmith/draft-quic-receive-ts