Summary: As in title. This doesn't actually send any frames, but implements basic support for the transport parameter and responding to the frames.
Reviewed By: yangchi
Differential Revision: D26134787
fbshipit-source-id: 2c48e01084034317c8f36f89c69d172e3cb42278
Summary: this param is passed to transport then ignored
Reviewed By: avasylev
Differential Revision: D26133327
fbshipit-source-id: 459dd0132185513215ba034f213d4137d7b56ba1
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: `QuicSocket::TransportInfo::packetsSpuriouslyLost` is not currently being set (always zero). Fixed this and renamed it to more closely match the other counters.
Differential Revision: D25999285
fbshipit-source-id: 34fd380a2e56f110234987e08f3a0220669afde8
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: It is time. Also rename resetPacingTokens -> reset since there's no tokens anymore.
Reviewed By: yangchi
Differential Revision: D25073121
fbshipit-source-id: da68134a02abd3bfea99ac6618c07504e2990fb7
Summary: As in title. These actually show up in profiles.
Reviewed By: avasylev
Differential Revision: D25288369
fbshipit-source-id: b2178f3cc446248bfd7129a6b4451d7c811fb593
Summary: If the onByteEventCanceled callback changes the map then the iterator will no longer be valid.
Reviewed By: lnicco
Differential Revision: D25283060
fbshipit-source-id: e32bc8bc343d8d50785985a0bb607ab43188e464
Summary: Adds a top level API to set stream priorities, mirroring what is currently proposed in httpbis. For now, default all streams to the highest urgency, round-robin, which mirrors the current behavior in mvfst.
Reviewed By: mjoras
Differential Revision: D20318260
fbshipit-source-id: eec625e2ab641f7fa6266517776a2ca9798e8f89
Summary:
Knob Frames are supported only by peers who are on the QUIC::MVFST version.
Expose this information to the outside, so producers of Knob frames can check,
before constructing frames to exchange with the peer.
Reviewed By: bschlinker
Differential Revision: D24608694
fbshipit-source-id: 88a8ff892efc061ca755d3eeabaed83d27d8271f
Summary: Inform LifecycleObservers when an EventBase is attached or detached from a QuicSocket.
Reviewed By: bschlinker
Differential Revision: D24294254
fbshipit-source-id: a606ad5ae4d4cbe3095e6ae32d6c74713325ad08
Summary: To enable it without having to get a version
Reviewed By: lnicco
Differential Revision: D24220074
fbshipit-source-id: a7f4cd06580095192e13a77fb5bf79e86c2d1f14
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: Since the DCID is often empty for clients, this is otherwise not a very useful field.
Reviewed By: yangchi
Differential Revision: D23998639
fbshipit-source-id: b8949ca6913ed270e5ebd0a0c5335b224f817774
Summary:
After some experiments where probes are obviously causing congestion, it now
makes sense to pose some delay before sending the next probe. This is not
mentioned in the d6d spec, but in a related spec rfc4821 iirc, where consecutive
probes should have 1sec delay.
Reviewed By: mjoras
Differential Revision: D23910766
fbshipit-source-id: dcf5d05c4590489be503563c98144e12c3987cff
Summary: As a drive-by: fixed a bug where I didn't cancel the pending event when timeouts expire.
Reviewed By: mjoras
Differential Revision: D23910767
fbshipit-source-id: 233e590c17a6c6b7a5f4a251bd8f78f6dfae3c0b
Summary:
This glues together the d6d lifecycle via probe timeout and raise timeout.
Had to put these two timeouts in the base transport because it has all the
necessary accountings (e.g. check close state, process callbacks) that should
happen before scheduling timeouts.
Other notable changes (included here because code is simple):
- Keep track of d6d probes in loss state. Upon second thought, it makes more
sense because we are reducing the available bandwidth as a result of sending
probes anyway. And not tracking them imposes a delay on congestion controller.
I think this does not violate the d6d spec's point of not penalizing congestion
window for d6d probes, because
- 1. we don't put losses of d6d probes in loss event. Therefore from the POV of
congestion controller, d6d probes never get lost.
- there will be at most kDefaultD6DMaxOutstandingProbes losses (i.e. 2)
that we don't tell congestion controller about. Even if those are actually
caused by congestion, it should have minimal impact because 2 is small and if there's really a congestion, the loss of normal packets should provide the signal.
- Pacing d6d probes
- Kick off d6d after a delay of 1s. This should filter out short-lived connections where probing is relatively expensive and less useful.
Reviewed By: mjoras
Differential Revision: D23736656
fbshipit-source-id: 8121fa8bcebab10a56a4e046c32c4e99ed6c3013
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 introduces a new extension frame, the KnobFrame, and an implementation.
The idea with Knobs is that they are arbitrary key-values within a namespace that can be transmitted to the peer at any time. They provide an alternative to transport settings, by eschewing the standard transport setting space entirely.
The idea is simple, each knob has a "knobspace", "id", and value. The knobspace is a namespace in which the knob has semantic meaning (e.g. 0xfaceb00). The id and value are just that, arbitrary identifiers and values.
On receiving a knob it is the application's reponsibility to deal with it.
Reviewed By: mjoras
Differential Revision: D23601468
fbshipit-source-id: 63e5e4a7bdb76e11e8c952f1234f512a629ef348
Summary:
There are situations where application needs to know how many packets were transmitted for a stream on certain byte range. This diff provides *some* level of that information, by adding the `cumulativeTxedPackets` field in `StreamLike`, which stores the number of egress packets that contain new STREAM frame(s) for a stream.
~~To prevent double-counting, I added a fast set of stream ids.~~
Application could rely on other callbacks (e.g. ByteEventCallback or DeliveryCallback) to get notified, and use `getStreamTransportInfo` to get `packetsTxed` for a stream.
Reviewed By: bschlinker, mjoras
Differential Revision: D23361789
fbshipit-source-id: 6624ddcbe9cf62c628f936eda2a39d0fc2781636
Summary: ^ This might increase readability. e.g. "cbs->offset" is more readable than "cbs->first".
Reviewed By: bschlinker, yangchi
Differential Revision: D23361655
fbshipit-source-id: 811eb4439c2c717059ec5f0d15255ec5ede9a2b8
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: We have an API behavior where setReadCalback will issue a StopSending on behalf of the app. This is useful but has confusing semantics as it always defaults to GenericApplicationError::NO_ERROR. Instead let the error be specified as part of the API.
Reviewed By: yangchi, lnicco
Differential Revision: D23055196
fbshipit-source-id: 755f4122bf445016c9b5adb23c3090fc23173eb9
Summary:
LOG an error and fallback to no pacing. This diff also stops
supporting automaticaly set pacingEnabled to true when BBR is enabled.
Reviewed By: mjoras
Differential Revision: D22875904
fbshipit-source-id: f8c8c9ea252f6e5e86f83174309b159ce93b3919
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 is possible that when becoming app-limited we will have collected some extra pacing tokens. When we finally become un-app-limited, these tokens will cause us to write a large burst all at once.
Avoid this by explicitly resetting the tokens when we have new data to write, effectively restarting pacing.
Reviewed By: yangchi
Differential Revision: D22669645
fbshipit-source-id: 9415303952c96f12136a63c0d773255e28dc151c
Summary:
Our current timer compenstation works as following:
Pacer schedule write -> Pacer marks scheduled time T0-> timer fires -> Pacer uses (now - T0 + writeInterval) to calculate burst size -> transport writes burst size amount of data at time T1 -> pacer schedules again.
This diff changes to:
Pacer scheduleWrite -> timer fires -> Pacer uses (now - previous T1' + writeInteral) to calculate burst size -> transport writes burst size amount of data at T1 -> pacer schedules again
because T1' < T0 < T1, this compensates the timer more.
With higher compensation from timer interval calculation, the `tokens_ += batchSize_;` code inside refreshPacingRate is removed in this diff.
Reviewed By: yangchi
Differential Revision: D22532672
fbshipit-source-id: 6547298e933965ab412d944cfd65d5c60f4dced7
Summary: We don't know if this pointer remains valid after the callback.
Reviewed By: yangchi
Differential Revision: D22709445
fbshipit-source-id: 7802ab08052b06af0268652a44a080d9d9673122
Summary:
Adds support for timestamping on TX (TX byte events). This allows the application to determine when a byte that it previously wrote to the transport was put onto the wire.
Callbacks are processed within a new function `QuicTransportBase::processCallbacksAfterWriteData`, which is invoked by `writeSocketDataAndCatch`.
Reviewed By: mjoras
Differential Revision: D22008855
fbshipit-source-id: 99c1697cb74bb2387dbad231611be58f9392c99f
Summary:
Introduces framework for handling callbacks when a `ByteEvent` occurs.
- Adds `ByteEventCallback`, a new class that can be used to notify a callback implementation about delivery (ACK) of a byte and is used in the subsequent diff for notifications about TX of a byte.
- Adds `ByteEvent` and `ByteEventCancellation`, both of which are used to communicate ancillary information when a ByteEvent happens to the callback implementation. This makes it easier to add additional fields (such as kernel timestamps, information about ACK delays, etc) in the future, without requiring the function signatures of all callback implementations to be updated.
- For the moment, `DeliveryCallback` inherits from `ByteEventCallback` so that existing implementations do not need to be adjusted; `DeliveryCallback` implements `ByteEventCallback`'s methods and transforms the received `ByteEvents` into the expected callback. I will deprecate `DeliveryCallback` in a subsequent diff.
- Refactors existing logic for handling delivery callbacks to be generic and capable of handling different types of byte events. This allows the same logic for registering and cancelling byte events to be reused for ACK and TX timestamps; custom logic is only required to decide when to trigger the callback.
Reviewed By: mjoras
Differential Revision: D21877803
fbshipit-source-id: 74f344aa9f83ddee2a3d0056c97c62da4a581580
Summary:
Adds `QuicSocket::InstrumentationObserver`, an observer that can be registered to receive various transport events.
The ultimate goal of this class is to provide an interface similar to what we have through TCP tracepoints. This means we need to be able to register multiple callbacks.
- Initially, the first event exposed through the callback is app rate limited. In the future, we will expose retransmissions (which are loss + TLP), loss events (confirmed), spurious retransmits, RTT measurements, and raw ACK / send operations to enable throughput and goodput measurements.
- Multiple callbacks can be registered, but a `folly::small_vector` is used to minimize memory overhead in the common case of between 0 and 2 callbacks registered.
- We currently have a few different callback classes to support instrumentation, including `QuicTransportStatsCallback` and `QLogger`. However, neither of these meet our needs:
- We only support installing a single transport stats callback and QLogger callback, and they're both specialized to specific use cases. TransportStats is about understanding in aggregation how often an event (like CWND limited) is occurring, and QLogger is about logging a specific event, instead of notifying a callback about an event and allowing it to decide how to proceed.
- Ideally, we can find a way to create a callback class that handles all three cases; we can start strategizing around that as we extend `InstrumentationObserver` and identify overlap.
Differential Revision: D21923745
fbshipit-source-id: 9fb4337d55ba3e96a89dccf035f2f6978761583e
Summary:
Notify connection callback implementation when the transport becomes app rate limited
- The callback implementation is not required to define a handler for this callback (not pure virtual).
- D21923745 will add an instrumentation callback class that can be used to allow other components to receive this signal. I'm extending to the connection callback because the overhead seems low, and we already have the connection callback registered for `HQSession`.
Reviewed By: mjoras, lnicco
Differential Revision: D21923744
fbshipit-source-id: 153696aefeab82b7fd8a6bc299c011dcce479995
Summary:
Adds `QuicSocket::LifecycleObserver`, an observer that is notified when a socket is closed and destroyed.
- Can be used by instrumentation that ties its lifetime to that of the socket.
- Multiple observer can be registered, but a `folly::small_vector` is used to minimize memory overhead in the common case of 0 - 2 being registered.
- `folly::AsyncSocket` has a matching interface being added (D21613750), so instrumentation can follow the same paradigm for both QUIC and TCP.
- In the future, will extend to also be triggered when a transport becomes connected, similar to what we have for `AsyncSocket`.
Reviewed By: mjoras
Differential Revision: D21653651
fbshipit-source-id: 0aa374cc0dd9b9c11d81b49597326bd53264531d
Summary: Without this option the read callbacks come in an somewhat random order based on stream ID. This enforces ordering by stream ID, which is useful for some applications.
Reviewed By: yangchi
Differential Revision: D22442837
fbshipit-source-id: cd97de9760deff7b19841f46e23da2004df31492
Summary:
Callbacks notifying that max stream limit has been increased and new streams can be created.
This get triggered every time limit gets bumped, doesn't track if limit was previously exhausted or not.
Reviewed By: mjoras
Differential Revision: D22339814
fbshipit-source-id: 6bcd0b52adb61e1ac440d11559dc8544ad7aa1ac
Summary:
The problem with Ping being a simple frame:
(1) All SimpleFrames are in the same scheduler. So sending ping means we may
also send other frames which can be problematic if we send in Initial or
Handshake space
(2) Ping isn't retranmisttable. But other Simple frames are. So we are
certainly setting this wrong when we send pure Ping packet today.
That being said, there are cases where we need to treat Ping as retransmittable.
One is when it comes to update ack state: If peer sends us Ping, we may want to
Ack early rather than late. so it makes sense to treat Ping as retransmittable.
Another place is insertion into OutstandingPackets list. When our API user sends
Ping, then also add a Ping timeout. Without adding pure Ping packets into OP list,
we won't be able to track the acks to our Pings.
Reviewed By: mjoras
Differential Revision: D21763935
fbshipit-source-id: a04e97b50cf4dd4e3974320a4d2cc16eda48eef9
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
