Summary: Continuing the theme, this time with inplaceEncrypt. We have to catch the exceptions from Fizz for the bridge version.
Reviewed By: kvtsoy
Differential Revision: D75886829
fbshipit-source-id: f74d5ce73e242e20e4c62552748970b2d5d7cb79
Summary: Some old MVFST clients did not support key updates. This change prevents the server from initiating key updates to clients using the MVFST quic version until the client has initiated a key update. This ensures that key updates are triggered for clients that support them.
Reviewed By: mjoras
Differential Revision: D75022528
fbshipit-source-id: 7af2d65becbfefee347cbe98ccdfc01323cd153c
Summary:
This is an API break, but it should mostly be a manageable one. We want to be able to compile mvfst internally without exceptions, and folly::Optional is one dependency that makes this challenging. Additionally, we already have an imported secondary optional type for performance/struct size reasons, tiny-optional.
This second optional interface is mostly compatible in an API sense (including the use of std::nullopt) with std::optional. Thus our approach is to remove the dependency on folly::Optional, and offer a quic::Optional instead.
The next diff will properly vendor tiny-optional so that quic::Optional is an independent version of it.
Reviewed By: sharmafb, kvtsoy
Differential Revision: D74133131
fbshipit-source-id: 715f8bb5043ba3bb876cacfe54236887e0686b30
Summary:
When the transport is in priming mode it saves all packets instead of writing them on the wire, and feeds them to a callback for the caller to get the data.
Meta:
Priming mode is used to get all packets before 1-RTT cipher is available, in order for them to get replayed later.
Reviewed By: kvtsoy
Differential Revision: D71290230
fbshipit-source-id: 230650cb1e5901069dda4ef850c9c724bf33b6be
Summary:
- Fail the connection if the peer acks future packets.
- Skip a packet number randomly and fail if the peer acks it
- Account for the skipped packet in the reorder distance when detecting losses
- Account for the skipped packet when implicitly acking crypto streams
Reviewed By: mjoras, kvtsoy
Differential Revision: D73120636
fbshipit-source-id: 661e4b418f9f015d6de627b67e1cf4d1fd81bb27
Summary:
Previously,
* `RawBuf` was a typealias for `std::unique_ptr<folly::IOBuf>`
* `Buf` was a typealias for `folly::IOBuf`
In this diff,
* `Buf` is a typealias for `folly::IOBuf`
* `BufPtr` is a typealias for `std::unique_ptr<folly::IOBuf>`
Reviewed By: hanidamlaj
Differential Revision: D73206576
fbshipit-source-id: 454bf6ccfce3d6571e5e931889263ed98cc24af3
Summary: Continuing the theme. This removes it from client and server handshakes.
Reviewed By: kvtsoy
Differential Revision: D73335422
fbshipit-source-id: 262bad17c1ebd2bcef623b1185e38e6a63ec714b
Summary: Previously, we didn't support all combinations of (batching mode, data path type).
Reviewed By: mjoras
Differential Revision: D73280506
fbshipit-source-id: 752a1eda4f58a19f86410bff514415017ffdb383
Summary: More in the theme of returning Expected instead of throwing. For the folly case, we keep the try/catches in there and translate to Expected. For Libev, we convert directly to Expected.
Reviewed By: kvtsoy
Differential Revision: D73217128
fbshipit-source-id: d00a978f24e3b29a77a8ac99a19765ae49f64df8
Summary: This introduces a more generic typealias so that we can, for instance, write `BufHelpers::createCombined` instead of `folly::IOBuf::createCombined`.
Reviewed By: jbeshay
Differential Revision: D73127508
fbshipit-source-id: d585790904efc8e9f92d79cbf766bafe0e84a69f
Summary:
This adds the new priority queue implementation and a TransportSetting that controls whether it should be used or not. The default is still the old priority queue, so this diff should not introduce any functional changes in production code.
One key difference is that with the new queue, streams with new data that become connection flow control blocked are *removed* from the queue, and added back once more flow control comes. I think this will make the scheduler slightly more efficient at writing low-priority loss streams when there's high-pri data and no connection flow control, since it doesn't need to skip over those streams when building the packet.
If this diff regresses build size, D72476484 should get it back.
Reviewed By: mjoras
Differential Revision: D72476486
fbshipit-source-id: 9665cf3f66dcdbfd57d2199d5c832529a68cfac0
Summary: Continuing the theme, removing it from QuicInteger which ends up being in a lot of the write paths.
Reviewed By: kvtsoy
Differential Revision: D72757026
fbshipit-source-id: 99a6ab2caea8fb495b1cb466172611002968e527
Summary: As in title, this is more of a theme on adding an Expected return.
Reviewed By: kvtsoy
Differential Revision: D72579218
fbshipit-source-id: 25735535368838f1a4315667cd7e9e9b5df1c485
Summary: I started with the QuicStreamManager, but it turns out that the path from the manager up to the close path touches a LOT, and so this is a big diff. The strategy is basically the same everywhere, add a folly::Expected and check it on every function and enforce that with [[nodiscard]]
Reviewed By: kvtsoy
Differential Revision: D72347215
fbshipit-source-id: 452868b541754d2ecab646d6c3cbd6aacf317d7f
Summary: Cache the negotiated config for what ACK type to write and which fields to use once the peer transport parameters are available. This avoids computing the config with every ack frame being written.
Reviewed By: sharmafb
Differential Revision: D70004436
fbshipit-source-id: 79354f5137c77353c3a97d4c41782a700622e986
Summary: This was added to not bundle ACKs with stream frames. Don't special case those and also disabling opportunistic ACKing for all write reasons.
Reviewed By: jbeshay
Differential Revision: D66514392
fbshipit-source-id: f2657a5c06ea8ae37b8c8eacd04c5a3b8ac75390
Summary: In `writeCryptoAndAckDataToSocket`, add an additional `writeProbingDataToSocket` call at the end that is limited to the number of CRYPTO frame-containing packets just written, gated by the new `TransportSetttings` field `immediatelyRetransmitInitialPackets`.
Reviewed By: mjoras
Differential Revision: D64485616
fbshipit-source-id: f0927a3796767700fd46673195e1cd4e1bbbcbeb
Summary: This kinda makes sense to just use as a default.
Reviewed By: kvtsoy, sharmafb
Differential Revision: D64066392
fbshipit-source-id: 0915f163c0483af6bec014bde61e82b6ee2ac6cb
Summary: As in title. Without this the keepalive option is less effective since a single PING packet loss can cause issues.
Reviewed By: kvtsoy
Differential Revision: D63397494
fbshipit-source-id: 7ef6b6f54189609e3a96409ac9c035c475dc0569
Summary: This is my second attempt at D61871891. This time, I ran `xplat/cross_plat_devx/somerge_maps/compute_merge_maps.py`, which generated quic/somerge_defs.bzl
Reviewed By: kvtsoy
Differential Revision: D61975459
fbshipit-source-id: bec62acb2b400f4a102574e8c882927f41b9330e
Summary: `PacketEvent` is a very inaccurate and misleading name. We're basically using this as an identifier for cloned packets, so `ClonedPacketIdentifier` is a much better.
Reviewed By: kvtsoy
Differential Revision: D61871891
fbshipit-source-id: f9c626d900c8b7ab7e231c9bad4c1629384ebb77
Summary:
**Context**
The `BufAccessor` is used to access a contiguous section of memory. Right now, it works with a `Buf` under the hood.
**Overall plan**
The plan is to change the `BufAccessor` to use a `uint8_t*` instead. Since we're using section of contiguous memory, there's no need to use a chained buffer abstraction here. This'll move us closer to deprecating the usage `folly::IOBuf`.
**What this diff is doing**
Most use cases of the `BufAccessor` look like the following:
```
auto buf = bufAccessor.obtain();
// Do something with buf, like calling trimEnd
bufAccessor.release(buf)
```
I'm adding APIs to the `BufAccessor` so that there's no need to `obtain()` and `release()` the `Buf`. We'd instead just call an API on the `BufAccessor`, which would call that same API on the underlying `folly::IOBuf`. Later on, we'll change the `BufAccessor` to use a `uint8_t*` under the hood.
I'm currently leaving in the `obtain()`, `release()`, and `buf()` APIs because Fizz and the AsyncUDPSocket expect `folly::IOBuf` as inputs in many of their APIs. Once those callsites are migrated off `folly::IOBuf`, we can remove these APIs.
Reviewed By: mjoras
Differential Revision: D60973166
fbshipit-source-id: 52aa3541d0c4878c7ee8525d70ac280508b61e24
Summary:
Timely reaction to congestion requires relaying any CE marks to the sender as soon as possible.
This change schedules an ack to be sent whenever incoming packets are received with CE marks. This will only happen when the readEcnOnIngress option is enabled.
Reviewed By: mjoras
Differential Revision: D58423959
fbshipit-source-id: 30f8cf8b11d0446985c2d87d7df67c24c0d5afdf
Summary: Useful to track so we can optimize it.
Reviewed By: kvtsoy
Differential Revision: D58196435
fbshipit-source-id: c15c409f998430fd0c6acde0539d0345123a9e15
Summary: We have a lot of optionals that are either integral values or std::chrono::microseconds. These end up wasting memory, where we can instead store sentinel values to encode whether the value is there or not. This reduces the effective range of the type by one value, but that is an acceptable tradeoff.
Reviewed By: kvtsoy
Differential Revision: D57684368
fbshipit-source-id: b406b86011f9b8169b6e5e925265f4829928cc63
Summary:
The idea here is to make it so we can swap out the type we are using for optionality. In the near term we are going to try swapping towards one that more aggressively tries to save size.
For now there is no functional change and this is just a big aliasing diff.
Reviewed By: sharmafb
Differential Revision: D57633896
fbshipit-source-id: 6eae5953d47395b390016e59cf9d639f3b6c8cfe
Summary:
Remove cmsgs from each outstanding packet. Storing this as a map is excessive. Instead store it as a packed enum. This supports one kind of mark per packet.
If we ned to we could in the future support multiple types of marks per packet by using a bitset-style flag, e.g. each bit index representing a different mark. For now just use an enum.
Reviewed By: jbeshay
Differential Revision: D57397865
fbshipit-source-id: 6d34215c9d7e39537c44c6c304a8ce3a5883541e
Summary: This is effectively an unused field encoding duplicated information, but it was widespread.
Reviewed By: kvtsoy
Differential Revision: D57289922
fbshipit-source-id: ca1499e2576e5ae28e3880b865a29c2b8d9a3d1b
Summary:
The existing batch writers do not handle failed writes to the AsyncUDPSocket. A packet that fails to be written is detected as a packet loss later when feedback is received from the peer. This negatively impacts the congestion controller because of the fake loss signal, and artificially inflates the number of retransmitted packets/bytes.
This change adds a new batch writer (SinglePacketBackpressuretBatchWriter) that retains the buffers when a write fails. For subsequent writes, the writer retries the same buffer. No new packets are scheduled until the retried buffer succeeds.
Notes:
- To make sure that retry writes are scheduled, the write callback is installed on the socket when a buffer needs to be retried.
- The retries are for an already scheduled packet. The connection state reflects the timing of the first attempt. This could still have an impact on rtt samples, etc. but it this is a milder impact compared to fake losses/retranmissions.
- Any changes outside of the batch writer only impact the new batch writer. Existing batch writers do not use the fields and are not affected by the changes in this diff.
Reviewed By: kvtsoy
Differential Revision: D57597576
fbshipit-source-id: 9476d71ce52e383c5946466f64bb5eecd4f5d549
Summary:
The time between iterations is not significant, so we can just call `Clock::now` once in the beginning and reuse the same value.
I ran a canary with some counters to get an idea of the amount of time between the start of the first iteration and the end of the last iteration (see D57510979), and:
* Edge p100: 1500 us
* olb p100: 1900 us
* Edge p99: 413 us
* olb p99: 396 us
The wins we're seeing are 0.13% relative CPU.
Reviewed By: kvtsoy
Differential Revision: D57594650
fbshipit-source-id: 9d0f827564179745cd83eb6ca211df68d3f23f8b
Summary: This setting is no longer needed.
Reviewed By: mjoras
Differential Revision: D57112554
fbshipit-source-id: 4720dd864f24ac21a775419522254195c5ea215f
Summary:
std::deque by default allocates a large block on the heap for managing its state. This has a fixed memory cost both per connection (because of the crypto streams) and per stream. CircularDeque by comparison does not have this overhead and is only 32 bytes per structure.
For example:
```
"size": 656,
"name": "readBuffer",
"typePath": ["a0", "conn_", "ptr_val", "cryptoState", "ptr_val", "initialStream", "readBuffer"],
"typeNames": ["std::deque<quic::StreamBuffer, std::allocator<quic::StreamBuffer>>"],
```
This should save about 6kB per connection with no streams, and additional memory per stream.
Reviewed By: jbeshay, hanidamlaj
Differential Revision: D56578219
fbshipit-source-id: ab2b529fa9a4169bea6862b11ccbf178c6f5abb1
