Summary:
- The current mvsft implementation only checks against the duration threshold to establish persistent congestion. However, according to the spec, persistent congestion should only be established if both the threshold duration is exceeded and there are no acked packets between the send times of the lost packets.
- In addition to the logic already present in `isPersistentCongestion()`, `isPersistentCongestionExperimental()` validates that no acked packet exists between the send times of the two lost ack-eliciting packets.
Reviewed By: mjoras
Differential Revision: D30117835
fbshipit-source-id: 8809ccda761fe67b4cf51abbf2d5c0d5a4ed2b38
Summary:
Introruced a new DetailsPerStream struct inside the outstanding packet metadata
so we can easily track all streams carrying app data within a packet and each
stream's bytes sent (both new and total).
With this, consumers can easily deduce the number of retransmitted bytes per
stream using the StreamDetails members:
newStreamBytesSent - streamBytesSent.
Reviewed By: bschlinker
Differential Revision: D28063916
fbshipit-source-id: d915514f85f24f4889cfac2f7a66bfd2d6b0a4af
Summary:
some of the code was written when we thought we'd have a
per-connection sender. Now that the sender is per-stream, these code are
updated. It is a bit awkward though. The write() function doesn't know what
stream it will be writing. Scheduler knows it, but Scheduler isn't the right
place to decide when to flush() the sender. So now the return type of packet
scheduling will include a raw pointer to the sender.
Reviewed By: mjoras
Differential Revision: D27869558
fbshipit-source-id: 2490d364d79d22db6f66eb82b194cfd51c182844
Summary: Keep on server for now but disallow it in code for the client.
Reviewed By: yangchi
Differential Revision: D27726584
fbshipit-source-id: c567d9db82c36b6e60d438d839709f0330b8db50
Summary:
As before we will now aggressively send probes on all spaces with probes available when the PTO timer fires.
This time with more unit tests and some bug fixes.
Reviewed By: yangchi
Differential Revision: D27338523
fbshipit-source-id: 8a9ccb90ed691e996fab4afa2f132c0f99044fbc
Summary:
Previously, we maintained state and counters to count both, header and body
bytes together. This commit introduces additional counters and state to keep
track of just the body bytes that were sent and acked etc. Body bytes received
will be implemented later.
Reviewed By: bschlinker
Differential Revision: D27312049
fbshipit-source-id: 33f169c9168dfda625e86de45df7c00d1897ba7e
Summary: As in title. There's a bug here somewhere with empty write loops we need to find.
Reviewed By: yangchi
Differential Revision: D27279100
fbshipit-source-id: e1d26fbf8d6df1590d464a6504a8b940b46794e0
Summary:
Previously we would only send probes for the first space which had one available, i.e. Initial before Handshake before AppData. Since we only have one PTO timer this can lead to situations where we perpetually probe with only Initials, which can significantly delay the handshake if we should have probed with Handshakes.
With this diff we will keep the single PTO timer but aggressively write more probes from all spaces if they are available.
Additionally this refactors some counters into EnumArrays
Reviewed By: yangchi
Differential Revision: D27235199
fbshipit-source-id: ef3614a833bf0f02f5806846a1335fa7ac2a4dc8
Summary:
- Adds counter of the number of ack-eliciting packets sent; useful for understanding lost / retransmission numbers
- Adds the following to `OutstandingPacketMetadata`
- # of packets sent and # of ack-eliciting packets sent; this enables indexing of each packet when processed by an observer on loss / RTT event, including understanding its ordering in the flow of sent packets.
- # of packets in flight; useful during loss analysis to understand if self-induced congestion could be culprit
- Fixes the inflightBytes counter in `OutstandingPacketMetadata`; it was previously _not_ including the packets own bytes, despite saying that it was.
Right now we are passing multiple integers into the `OutstandingPacket` constructor. I've switched to passing in `LossState` to avoid passing in two more integers, although I will need to come back and clean up the existing ones.
Differential Revision: D25861702
fbshipit-source-id: e34c0edcb136bc1a2a6aeb898ecbb4cf11d0aa2c
Summary:
Extended instrumentationObserver to record packets that were spuriously lost due to reordering or timeout.
Depends on https://github.com/facebookincubator/mvfst/issues/185
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/186
Test Plan:
buck test //quic/api/test/...
buck test //quic/loss/test/...
buck test //quic/state/test/...
Proxygen canary on ELB and FLB
Reviewed By: bschlinker
Differential Revision: D24072374
Pulled By: sridharsrinivasan86
fbshipit-source-id: 2a22eeae7eb5c1bab405e8c5b7f4b96dff5afb59
Summary:
Given the large number of callbacks that are being triggered from the Observer
this change makes it possible to enable through a simple config, just the
subset of callbacks that a consumer is interested in receiving.
Observer and Socket Lifecycle callbacks are enabled by default, they are not
configurable.
Reviewed By: bschlinker
Differential Revision: D25879382
fbshipit-source-id: abe79ed92e958ddc96475c347f8ec7204400cdfa
Summary:
We were using the LifecycleObserver and InstrumentationObserver classes
separately, to generate and receive callbacks.
This change migrates both these to use the unified Observer callback class and
adjusts the unit tests.
Reviewed By: bschlinker
Differential Revision: D25845845
fbshipit-source-id: c489400f5d70bccadbcc1d957136c5ade36b65ff
Summary:
I think this should just work without the trailing `_E`. It was added
when we mixed up our own union based variant and boost::variant. Some compiler
flags didn't like that. Now we no longer have mixed up cases, this should be
fine
Reviewed By: lnicco
Differential Revision: D25589393
fbshipit-source-id: 6430dc20f8e81af0329d89e6990c16826da168b8
Summary:
We currently have a count of the number of packets retransmitted, and the number of packets marked as lost spuriously, but we do *not* have the total number of packets sent or lost, which is necessary to calculate statistics such as % of packets retransmitted % of losses that are spurious.
At the moment, we do not count loss events for D6D packets. Will likely add a separate counter for those in a subsequent diff.
Reviewed By: xttjsn
Differential Revision: D25540531
fbshipit-source-id: 80db729eb8c91f7805d342f4aab302a5b3ca4347
Summary: This reduces dependencies for both testing and instrumentation.
Reviewed By: mjoras
Differential Revision: D23997313
fbshipit-source-id: 5eb3a790c7bb2569dc1e941e3911ad4aac4e9258
Summary:
Giving more access to the current state of the connection by exposing the socket.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/185
Reviewed By: yangchi
Differential Revision: D23924861
Pulled By: bschlinker
fbshipit-source-id: 16866a93e58b6544e25d474a51ca17fab9ffdc55
Summary:
Due to high number of RTT samples I refactored the OutstandingPacket to
split the packet data and packet metrics. We know have access to metrics
without the need of saving the packet data.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/178
Reviewed By: mjoras
Differential Revision: D23711641
Pulled By: bschlinker
fbshipit-source-id: 53791f1f6f6e184f37afca991a873af05909fbd2
Summary:
This is a simple PMTU blackhole detection method by counting packet losses
within a short window. If the cumulative packet losses exceed the threshold,
we reduce udpSendPacketLen to base pmtu. Start with a window of 5 seconds and
threshold of 5 as a first attempt.
Reviewed By: mjoras
Differential Revision: D23734194
fbshipit-source-id: 457c8204e6625bd13b366c7b0c6128e340118d57
Summary:
According to the spec, loss of d6d probe packet should not affect congestion
control, but AFAIU its ack should be considered a normal ack and has all the
implications of an ack (e.g. sample srtt, increment largest ack num).
Additionally, although d6d probe uses ping frame, neither sending a d6d probe
nor receiving the ack should have any bearing on either pendingEvents.sendPing
or pendingEvents.cancelPingTimeout.
To differentiate a d6d probe from a normal packet in tx/rx path, the isD6DProbe
flag is added. I also added a new struct to store d6d specific states (1 field
in this diff, more to come in subsequent diffs). In updateConnection, we
identify a d6d probe by comparing the packet sequence num with the sequence num
stored in the lastProbe field of the d6d state.
Reviewed By: mjoras
Differential Revision: D22551400
fbshipit-source-id: 85ec30c185666c3d5cf827bf03b4f92e6f22d4ec
Summary:
`InstrumentationObserver` is owned by `QuicSocket` now. However, to capture packet loss signal, we need it in `QuicConnectionState`. So, I refactored the code a little bit to make this easy. Changes in this code:
(a) Moved the definition of `InstrumentationObserver` and `LifeCycleObserver` to
a separate header `Observer.h`.
(b) Moved the vector of `InstrumentationObserver`s from `QuicSocket` to `QuicConnectionState`.
(c) Added a callback in `conn->pendingCallbacks` when a packet loss is detected
Reviewed By: bschlinker
Differential Revision: D23018569
fbshipit-source-id: e70d954839bdb70679ecd52d2bd1a6a6841f6778
Summary: This was probably a premature optimization and introduces complexity for dubious gain. Additionally a sequence of losses could potentially cause multiple updates to be delayed.
Reviewed By: yangchi
Differential Revision: D23628058
fbshipit-source-id: d6cf70baec8c34f0209ea791dadc724795fe0c21
Summary:
This is following a similar pattern than what was done for the client side.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/160
Reviewed By: yangchi
Differential Revision: D23560951
Pulled By: xttjsn
fbshipit-source-id: 351417cbfa3230112fff4c4de59b307f88389cf6
Summary:
It is useful to be able to account for when we detect packets as lost but actually receive an ACK for them later. This accomplishes that by retaining the packets in the outstanding packet list for a certain amount of time (1 PTO).
For all other purposes these packets are ignored.
Reviewed By: yangchi
Differential Revision: D22421662
fbshipit-source-id: 98e3a3750c79e2bcb8fcadcae5207f0c6ffc2179
Summary:
duration_cast round towards zero. The diff uses folly::chrono::ceil
rather than std::chrono::ceil since we still need to compile with c++14
Differential Revision: D22870632
fbshipit-source-id: 18439488e879164807b1676a0105073348800412
Summary:
It seems there is a possibility of keeping a early retransmit timer
around after the packet sets it is no longer present. It's a little hard
to get to that condition though. See the test case.
Reviewed By: mjoras
Differential Revision: D22194495
fbshipit-source-id: 50cf2dcb2afe00db14c521126d338057605aa907
Summary:
On loss timer, currently we knock all handshake packets out of the OP
list and resend everything. This means miss RTT sampling opportunities during
handshake if loss timer fires, and given our initial loss timer is likely not a
good fit for many networks, it probably fires a lot.
This diff keeps handshake packets in the OP list, and add packet cloning
support to handshake packets so we can clone them and send as probes.
With this, the handshake alarm is finally removed. PTO will take care of all
packet number space.
The diff also fixes a bug in the CloningScheduler where we missed cipher
overhead setting. That broke a few unit tests once we started to clone
handshake packets.
The writeProbingDataToSocket API is also changed to support passing a token to
it so when we clone Initial, token is added correctly. This is because during
packet cloning, we only clone frames. Headers are fresh built.
The diff also changed the cloning behavior when there is only one outstanding
packet. Currently we clone it twice and send two packets. There is no point of
doing that. Now when loss timer fires and when there is only one outstanding
packet, we only clone once.
The PacketEvent, which was an alias of PacketNumber, is now a real type that
has both PacketNumber and PacketNumberSpace to support cloning of handshake
packets. I think in the long term we should refactor PacketNumber itself into a
real type.
Reviewed By: mjoras
Differential Revision: D19863693
fbshipit-source-id: e427bb392021445a9388c15e7ea807852ddcbd08
Summary: 0 is now a valid packet number, so we should make these optional. In cases where they are needed to construct packet builder, it should be safe to use 0 as default since it's only used for computing `twiceDistance` in PacketNumber.cpp.
Reviewed By: yangchi
Differential Revision: D21948454
fbshipit-source-id: af9fdc3e28ff85f1594296c4d436f24685a0acd6
Summary:
as title. This also fixes the handshake alarm QLog. It logs before we
increased the handshake alarm count
Reviewed By: mjoras
Differential Revision: D20831207
fbshipit-source-id: c9a555884ec1c5737180c64b686793df0351a488
Summary:
Currently the packet builder contructor will encode the packet
builder. This is fine when the builder creates its own output buffer. If later
on we decides not to use this builder, or it fails to build packet, the buffer
will be thrown away. But once the builder uses a buffer provided by caller, and
will be reused, we can no longer just throw it away if we decide not to use
this builder. So we have to delay the header encoding until we know we will use
the builder.
This is still not enough to solve the case where we want to use this builder,
it builds, then it fails . For that, we will need to retreat the tail position
of the IOBuf.
Reviewed By: mjoras
Differential Revision: D21000658
fbshipit-source-id: 4d758b3e260463b17c870618ba68bd4b898a7d4c
Summary: This implements the handshake done signal and also cipher dropping.
Reviewed By: yangchi
Differential Revision: D19584922
fbshipit-source-id: a98bec8f1076393b051ff65a2d8aae7d572b42f5
Summary:
our convention has always been to put the mock in the test dir under
the real class
Reviewed By: lnicco
Differential Revision: D20104476
fbshipit-source-id: 5215ffc9af7a6d7a5ac41109723a71f68f852af7
Summary: no more surprises in upper layer
Reviewed By: mjoras
Differential Revision: D19976510
fbshipit-source-id: 3487e9aa2cb28d7bc748f13bc2bbc393216b4a8a
Summary:
In a recent diff this was mistakenly changed to be the earliest sent
time, which makes the loss alarm duration much shorter, then we end up firing
them a lot.
Reviewed By: mjoras
Differential Revision: D19826034
fbshipit-source-id: e190d30655533fdb220e92eddc5754fb9abd007b
Summary:
This diff:
1) introduces `EnumArray` - effectively an `std::array` indexed by an enum
2) changes loss times and `lastRetransmittablePacketSentTime` inside `LossState` to be an `EnumArray` indexed by `PacketNumberSpace`
3) makes the method `isHandshakeDone()` available for both client and server handshakes.
4) uses all those inputs to determine PTO timers in `earliestTimeAndSpace()`
Reviewed By: yangchi
Differential Revision: D19650864
fbshipit-source-id: d72e4a0cf61d2dcb76f0a7f4037c36a7c8156942
Summary: Some test cases used FileQLogger to verify that correct events have been logged. MockQLogger allows to check what events are logged without using an in-memory store like in FileQLogger. Migrated QuicLossFunctionsTest and QuicTransportTest from FileQLogger to MockQLOgger to simplify logic and cut dependencies.
Reviewed By: yangchi
Differential Revision: D19426423
fbshipit-source-id: eb1ae16a81656efd7c491eae790484c73dede8f3
Summary: Previously we were treating the loss buffer the same way we were the retransmission buffer. That is, each entry represented a single stream frame that was lost. There's actually no reason to do this as we treat it more like the write buffer when retransmitting. It can also lead to pathological cases where we write consecutive byte ranges as separate stream frames.
Reviewed By: yangchi
Differential Revision: D19240352
fbshipit-source-id: 94ffb397fa287688d986a4a91d531d85050d9f98
Summary:
Google shared they saw 1/4 working better than 1/8. In our own test,
we saw 10-50% retransmission reduction from this change without hurting HTTP
latency.
Reviewed By: bschlinker
Differential Revision: D19289939
fbshipit-source-id: 097cc52be4b858b242edcf52183b85391879e894
