Summary: Use WriteOptions instead of a single int to allow for txTime setting too
Reviewed By: mjoras
Differential Revision: D48218906
fbshipit-source-id: 00a1d4905b54ec0614860f9cdd8b58c9d6e6ef9a
Summary: Turns out these functions have been returning 0 for bytes written for a while. Nothing was using them (yet), so there wasn't any functional breakage.
Reviewed By: kvtsoy
Differential Revision: D46653336
fbshipit-source-id: 765b3363c1fc0729e8239d1293ddcc4ae4bb9c79
Summary:
As in title.
(Note: this ignores all push blocking failures!)
Reviewed By: hanidamlaj, lnicco
Differential Revision: D46111136
fbshipit-source-id: b5aec9f1daa32acd200dd1550eb0bb3a9a44a4f2
Summary:
This has been hardcoded to SRTT/25 for a long time. However, especially when using DSR this might not be the most appropriate since it can start to get close to real world SRTTs.
Control it via a knob, and also add the behavior such that setting it to 0 effectively disables the time limit.
Reviewed By: jbeshay
Differential Revision: D46084438
fbshipit-source-id: 7dc619fdff1bd5c3f8654c4936200e0340ef94f2
Summary:
When using ACK_FREQUENCY and high reordering thresholds, the normal tricks for drawing ACKs with PTOs are slightly defeated.
Thankfully we can get around this by issuing IMMEDIATE_ACKs when probing. The strategy is basically to replace one of the usual probes with an IMMEDIATE_ACK probe.
Also, include ACKs in these probes (and the PING probes), since there's not much reason not to.
Reviewed By: jbeshay
Differential Revision: D45965703
fbshipit-source-id: 9b98473312a239e17a0b8040f45d197b1a2c8484
Summary:
To get reliable packet destruction events, created a `OutstandingPacketWrapper` wrapper that wraps the current `OutstandingPacket` class, so callback functions can be added to the wrapper instead of the underlying object itself.
#### Why do we need this wrapper?
`std::deque::erase` does not guarantee that appropriate object destructors will be called (only that the number of destructions = number of objects erased). This is a real problem as packet destruction events are then no longer reliable (OutstandingPacket object is wrong!). The wrapper class handles this condition by detecting it in the move assignment constructor (called during erase) and calls the appropriate packet destruction callback before packets are moved. If we did the same fix in the old OutstandingPacket, we have to make sure the callback is called before all the fields of OutstandingPacket are moved - this is not scaleable. Hence a wrapper with the underlying object and a destruction callback function.
I also disabled copy construction for OutstandingPacket (otherwise we will get duplicate OnPacketDestroyed callbacks and cannot track packets reliably). Removing packet copies also improves performance. Some code changes (in tests mostly) are mostly in service of this particular change.
Reviewed By: bschlinker
Differential Revision: D43896148
fbshipit-source-id: c295d3c4dba2368aa66f06df5fc82b473a03fb4d
Summary:
LLVM-15 has a warning `-Wunused-but-set-variable` which we treat as an error because it's so often diagnostic of a code issue. Unused variables can compromise readability or, worse, performance.
This diff either (a) removes an unused variable and, possibly, it's associated code, or (b) qualifies the variable with `[[maybe_unused]]`, mostly in cases where the variable _is_ used, but, eg, in an `assert` statement that isn't present in production code.
- If you approve of this diff, please use the "Accept & Ship" button :-)
Reviewed By: luciang
Differential Revision: D42465115
fbshipit-source-id: 0379c82451b1902c185bc3b083c7ee68264b8977
Summary:
We don't need to carry these states after the handshake is confirmed, so make them pointers instead. This will facilitate adding a structure to the AckState for tracking duplicate packets.
(Note: this ignores all push blocking failures!)
Reviewed By: hanidamlaj
Differential Revision: D41626895
fbshipit-source-id: d8ac960b3672b9bb9adaaececa53a1203ec801e0
Summary:
Track the total (cumulative) amount of time that the connection has been application limited and store this information in `OutstandingPacketMetadata`. If this value is the same for two `OutstandingPacketMetadata` then we know that the transport did not become application limited between when those two packets were sent (or, less likely, the transport was application limited for less than one microsecond given the microsecond resolution of the timestamp).
We store the amount of time spent application limited instead of a count of the number of application limited events because the implications of being application limited are time dependent.
Tests show that we need to be able to inject a mockable clock. That's been an issue for some time; will work on in a subsequent diff.
Differential Revision: D41714879
fbshipit-source-id: 9fd4fe321d85639dc9fb5c2cd51713c481cbeb22
Summary: The code has changed since this was made an inlined vector. We now see higher cost from copy operations due to this rather than being able to move the data around.
Reviewed By: bschlinker
Differential Revision: D36151209
fbshipit-source-id: 0b10558c6bd8ebfea9bb960aac36a6c4044fc95f
Summary:
- PTOs should not be subject to congestion control limits
- Quickly recover from PTOs being writableBytesLimited by calling onPTOAlarm() as soon as we become unblocked
Reviewed By: mjoras
Differential Revision: D35480409
fbshipit-source-id: 51500db6fff17a7badefea8bda7f63141e97f746
Summary:
QE result post: https://fb.workplace.com/groups/646964542156536/permalink/1989360141250296/
We are going ahead with shipping this, with a default value of 32 for paddingModulo.
Leaving the transport knob settings still in place in case vips want to selectively disable the feature (not sure if that is the proper way to config that but I think so it is?)
Reviewed By: mjoras
Differential Revision: D34596608
fbshipit-source-id: 5603bb391113c29830f43a67e73c0f50154dcca1
Summary: - logging number of bytes the server sent during the handshake for insight.
Reviewed By: mjoras
Differential Revision: D33069800
fbshipit-source-id: e7e8f25183ee30de99e2971bae3c6b93882f6e63
Summary:
This is a bug that could prevent us from writing data if we ran out of connection flow control while we had lost data.
The last attempt missed a mistake in the scheduling of sequential priority streams.
Reviewed By: kvtsoy
Differential Revision: D33030784
fbshipit-source-id: e1b82234346a604875a9ffe9ab7bc5fb398450ed
Summary: As titled. Reduces confusion around `DetailsPerStream` containing a map that must be accessed through `get`, and instead just uses a similar approach as `quic::IntervalSet` — inheriting from the underlying data structure. While there's some disadvantages of this approach, they're minimized by only exposing a subset of accessors from the underlying data structure.
Reviewed By: afrind, mjoras
Differential Revision: D31887098
fbshipit-source-id: 51bdab35e6276926b6d85095d062971326be1b64
Summary: As titled. Reduces confusion, as we always expect this to be populated.
Differential Revision: D31887097
fbshipit-source-id: 153b05bae8abd559fe49d2c07c64d2ad0d92a809
Summary: This is causing empty write loops for some reason.
Reviewed By: jbeshay, lnicco
Differential Revision: D32990291
fbshipit-source-id: 2a183d591de54c7fe0ca54aea828a697a4cd9af0
Summary: This is an edge case we weren't handling properly. This can lead to situations where we run out of conn flow control, the peer hasn't received all data, and then we never write the data to trigger a flow control update.
Reviewed By: afrind
Differential Revision: D32887140
fbshipit-source-id: df5dfad8e0775ef43e5ca6ab98b8ca6b5987ce31
Summary: rename test local variables to be self documenting
Reviewed By: mjoras
Differential Revision: D32750782
fbshipit-source-id: 94ff5bbd34dbc804cd0229d8abd0ffd9891a44fc
Summary: Turns out the optimized builds of this test were running too fast and getting limited by flow control first rather than the intention which was to be limited by the time limit. Fix this by unlimiting them on flow control and giving some more headroom to hit the time limit.
Reviewed By: lnicco
Differential Revision: D32599440
fbshipit-source-id: 1ab2b15661cecdb44a62e3ca6048dc8424cceb1e
Summary: Otherwise we can end up in a situation where the non-DSR scheduler was limited by the loop time while the DSR scheduler is not, leading to an inconsistency.
Reviewed By: lnicco
Differential Revision: D32570027
fbshipit-source-id: f43d2517589c22bac0f2bb76626cc55c2a21fa5d
Summary:
Right now we are waiting for another write event to write datagrams.
Introduced a Write Reason for the case when only DATAGRAMS are pending
Reviewed By: mjoras
Differential Revision: D29091822
fbshipit-source-id: 0de6b88d93acae0ba240b9cdf9dbc8e74feaf5ac
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:
Track the number of stream bytes written to the wire and expose in `quic::TransportInfo`.
Implemented as two counters:
- `totalStreamBytesSent` is a count of all stream bytes written to the wire; the sum of the lengths of stream frames in all packets sent
- `totalNewStreamBytesSent` is a count of all *new* stream bytes written to the wire -- a stream byte is new if it has not been transmitted before; the sum of the lengths of stream frames that have never been transmitted before in all packets sent
We can derive the total number of stream bytes retransmitted as `totalStreamBytesSent - totalNewStreamBytesSent`. We may want to deprecate the existing `totalBytesRetransmitted` counter because the name of that counter does not make it clear that it is stream bytes, and because only includes some types of retransmissions.
While `totalNewStreamBytesSent` is already captured as `flowControlState.sumCurWriteOffset`, I am not reusing that counter here to avoid having a dependency on the information we track for flow control, and to allow all relevant information to be captured in `LossState` (where other relevant fields already exist).
Reviewed By: yangchi
Differential Revision: D28061085
fbshipit-source-id: 73486a4ba3fc8f12959f68702dc58e61fdc21b65
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: try to land this again without the compiler flags change
Differential Revision: D26958681
fbshipit-source-id: d00659aaf819dbb2942da8b41deab3d108a19f0f
Summary:
Update retransmission queue in the stream after BufferMeta is written,
for both new data and lost data cases.
Reviewed By: mjoras
Differential Revision: D26260987
fbshipit-source-id: 15a4fa17826426b4b972b63cf370fe791b3101ff
Summary:
Real stream data and BufferMeta represented data will be scheduled by different
schedulers. For that reason, this diff adds another PriorityQueue into the stream
manager.
Reviewed By: mjoras
Differential Revision: D26132498
fbshipit-source-id: c69cb671c9a9f975d82efab8f1244a2f3c6c9297
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:
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
