Summary:
Some of these are relevant to performance, others are just cleanups of the API usage.
The performance diffs are generally eschewing multiple lookups into the underlying structures, by reusing the iterator from `find`.
The cleanups are mostly getting rid of checks against `getStream`, which cannot return `nullptr`.
Reviewed By: yangchi
Differential Revision: D20987998
fbshipit-source-id: 1b95fd8b14da934fc921527aa858dcebf80ec8e9
Summary:
We always add stuff into peekable streams set since everything with a
read buffer is peekable. This makes the this set potentially big, and the
find_if quite expensive. For apps that isn't interested in peeking, this is a
waste of time.
Reviewed By: mjoras, lnicco
Differential Revision: D20972192
fbshipit-source-id: d696ded936140c622e019d608c72a646df405111
Summary: Doing an extra lookup here is wasteful.
Reviewed By: yangchi, lnicco
Differential Revision: D20848227
fbshipit-source-id: 8a1fee28597fae3cf1ac284a1bd781936ddff931
Summary:
This is a pretty confusing API behavior. This means that when reading from stream A, stream B could potentially be removed if it doesn't have a read callback set. This effectively means you cannot safely call read from onNew*Stream.
There's no real reason the check for a closed stream here, as the streams will get cleaned up from the onNetworkData or read looper calls.
Reviewed By: yangchi
Differential Revision: D20848090
fbshipit-source-id: 38eba9e5a24b6ddf5bf05ac75787dda9c001c1c6
Summary:
Right now we use un-sanitized error message on onConnectionError only.
This also uses it in app callbacks
Reviewed By: lnicco
Differential Revision: D20843327
fbshipit-source-id: c81896d41b712a7165ac6f6b381d3687ecca2a3a
Summary: It's not a great practice to leak the excpetion string via a conn close, but it is useful for the app to be able to report what the exception string was.
Reviewed By: yangchi
Differential Revision: D20628591
fbshipit-source-id: bf6eb5f33f516cec0034caed53da998643fcc120
Summary:
This ensures they are available to the whole stack rather than the transport only. The validator needs it in the server case, and will soon need it in the client case, so that seems appropriate to make it available.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/117
Reviewed By: yangchi
Differential Revision: D20536366
Pulled By: mjoras
fbshipit-source-id: a76d369c0a82b9be1f985aed1f33f7a6b338a2ae
Summary:
Currnetly there isn't a way for apps to unregister a pending
WriteCallbacks for a stream. resetStream() does that if the transport isn't in
Closed state. This diff adds such support even if transport is already in
Closed state. This solves the problem where app has a class that's both stream
ReadCallback and stream WriteCallback and readError would kill the callback
object itself. The new API gives such class a chance to remove itself from the
transport.
Reviewed By: mjoras
Differential Revision: D20545067
fbshipit-source-id: 81d9f025310769aadef062711a49adc47a0639d0
Summary:
This somewhat contrains what applications using mvfst can do with their errors, as it makes the semantics of onConnectionEnd vs onConnectionError tied to the value of GenericApplicationError::NO_ERROR.
For now, we believe this is fine, and fixes a case of connection close mis classification by HQSession.
Reviewed By: yangchi
Differential Revision: D20550139
fbshipit-source-id: eec7d90c33141bfa7f1280bf5b569818890d1130
Summary: Previously we would end up writing a non-application close when the application calls close(folly::none). This isn't correct, as those cases should be an application error with no error.
Reviewed By: afrind
Differential Revision: D20518529
fbshipit-source-id: fe069cccc32bd550fb3ec599694110955816993f
Summary: Resetting a stream has the semantics of abandoning all writes on that stream. As such, the application resetting the stream is an implicit signal that it does not care about further writes. It is reasonable then for an application to not expect further write callbacks to trigger.
Reviewed By: lnicco
Differential Revision: D20462859
fbshipit-source-id: b6701e6a262d618c5cd93fd1531095a134f6554e
Summary:
Similar to the exiting empty write loop callback. The new API will
trigger when we read from socket but back with empty hands.
Reviewed By: lnicco
Differential Revision: D20130432
fbshipit-source-id: 9b61310b4ea4c5c7999742c5a8761a831f20f7b7
Summary:
(1) The first change is the pacing rate calculation is simplified. It
removes the interval calculation and just uses the timer tick as the interval.
Then it calculates the burst size from there. For most cases these two
calculation should land at the same result, except when the
`cwnd < minBurstSize * tick / RTT`. In that case, the current calculation would
spread writes evenly across one RTT, assuming no new Ack arrives during the RTT;
while the new calculation uses the first a few ticks to finish the cwnd amount
of data.
(2) Then this diff changes how we compensate late timer. Now the pacer will
maintain a nextWriteTime_ and lastWriteTime_, which makes it easier to
calculate time elapsed since last write. Then each time writer tries to write,
it will be allowed to write timeElapsed * pacingRate. This is much more
intuitive than the current logic.
(3) The diff also adds pacing limited tracking into the pacer. An expected
pacing rate is cached when pacing rate is refreshed by congestion controller.
Then with packets sent out, Pacer keeps calculating the current send rate. When
the send rate is lower, Pacer sets pacingLimited_ to true. Otherwise false.
Only when the connection is not pacing limited, the lastWriteTime_ will be
packet sent time, otherwise it will be set to the last nextWriteTime_. In other
words: if the send rate is lower than expected, we use the expected send time
instead of real send time to calculate time elapsed, to allow higher late
timer compenstation, to give pacer a chance to catch up.
(4) Finally this diff removes the token collecting behavior in the pacer. I
think having tokens increaed, instead of reset, when an ack refreshes the pacing
rate or when we compensate late time, is quite confusing to some people. After
all the above changes, I found tperf can still sustain good throughput without
always increase tokens, and rally actualy gives even better results. So i think
we can remove this part of the pacer that's potentially very confusing to
people who don't know how we got there.
Reviewed By: mjoras
Differential Revision: D19252744
fbshipit-source-id: b83e4a01fc812fc52117f3ec0f5c3be1badf211f
Summary:
All instancesi of LIKELY and UNLIKELY probably should be removed. We will
add them back in if we see pathologies in performance profiles.
Reviewed By: mjoras
Differential Revision: D19163441
fbshipit-source-id: c4c2494d18ecfd28f00af1e68ecaf1e85c1a2e10
Summary:
Currently, before server generate the destination CID, we route packets with client's address, port and client's source connection ID. But now that client can use 0-len source connection ID, the different connections from the same client address and port will be routed to the same server connections.
This diff changes it to use client's initial destination connection ID as part of the routing key.
Reviewed By: udippant
Differential Revision: D19268354
fbshipit-source-id: 837f5bd2f1e3a74957afacf7aabad922b1719219
Summary:
In QuicExceptions, in the case where the toString method was able to statically determine the response strings, we simply return the string literals in a folly::StringPiece instead of unnecessarily copying them into std::string.
Some toString methods had some dynamically generated responses and thus could not be updated. Added a TODO explaining the fact.
Reviewed By: mjoras
Differential Revision: D19192117
fbshipit-source-id: d9e5f202f9bf240009e8b8fd16f337b0506fbeb0
Summary: This is sanitizing our error strings so that we do not leak them on the wire in connection close reasons.
Reviewed By: yangchi
Differential Revision: D18657317
fbshipit-source-id: 06cdd983fd2c9cade77f8410e124920e4cdfac59
Summary:
Previously we track them since we thought we can get some additional
RTT samples. But these are bad RTT samples since peer can delays the acking of
pure acks. Now we no longer trust such RTT samples, there is no reason to keep
tracking pure ack packets.
Reviewed By: mjoras
Differential Revision: D18946081
fbshipit-source-id: 0a92d88e709edf8475d67791ba064c3e8b7f627a
Summary:
In the current client code we read one packet, go back to epoll, and then read
another packet. This is not very efficient.
This changes it so that we can read multiple packets in one go from an epoll
callback.
This only performs changes on the client
Reviewed By: mjoras
Differential Revision: D18797962
fbshipit-source-id: 81be82111064ade4fe3a07b1d9d3d01e180f29f5
Summary: The state machine logic is quite abstruse, this modifies it to make it more readable.
Reviewed By: siyengar
Differential Revision: D18488301
fbshipit-source-id: c6fd52973880931e34904713e8b147f56d0c4629
Summary:
F14 should be faster and have lower memory urilization for near-empty sets and maps. For most H3 connections these are mosotly going to be near-empty, so CPU wins will likely be minimal.
For usecases that have extremely high numbers of streams, there are likely going to be CPU wins.
Reviewed By: yangchi
Differential Revision: D18484047
fbshipit-source-id: 7f5616d6d6c8651ca5b03468d7d8895d1f51cb53
Summary:
QLogConnectionMigrationEvent:
Allow observing client-side ConnectionMigration attempts (replacing the
socket), and observing the server-side changing the peer address it
is writing to.
QLogPathValidationEvent:
Allow observing successful/failed path validation attempts.
Success is considered as a correct PathResponse being returned.
A Failure is only published on the timeout expiring, not an invalid
PathChallenge frame being returned (we do not cancel this).
There are already QlogEvents for PathChallenge/PathResponse that
can be observed.
Reviewed By: JunqiWang
Differential Revision: D18340999
fbshipit-source-id: 512108f82a6e082021c0bd3254f108c128b17ba3
Summary: inspect PN, and when it reaches 2^62 -2 trigger a transport close through a pending event.
Reviewed By: yangchi
Differential Revision: D18239661
fbshipit-source-id: 1a218678099016693149e12ff121e2a39b95aecc
Summary: Enable pacing when CC is changed to BBR
Reviewed By: yangchi
Differential Revision: D18231888
fbshipit-source-id: a54b6313d089c2ae24ce6bf6ee56c4fe0d6b4722
Summary:
We currently pause all producers if the sum of the egress buffers of all transactions exceeds the write buffer limit. This turns out to be deterimental to prioritization.
Now, we pass the underlying transport pause state or connection flow control state back to the handlers. The previous diff in this stack introduces a per-stream buffer limit (64kb default). To limit total session buffer size, limit the number of concurrent streams or lower the per-stream limit.
Reviewed By: lnicco
Differential Revision: D17097138
fbshipit-source-id: 9025c5be8b318963311c3aaad9ee9a03c0e2265e
Summary: Use the custom variant type for errors.
Reviewed By: yangchi
Differential Revision: D17826935
fbshipit-source-id: 2bf0c3e1cc8ca84b504d201fd6c2a4266878b715
Summary:
CC Type is not exposed anywhere else. This could be the source of truth and will be useful for logging.
Also added a helper to convert the enum to string.
Reviewed By: yangchi
Differential Revision: D17566664
fbshipit-source-id: 1e0e887a7c23617b174b240f5c636f6dcdfd42c4
Summary:
Currently the lower bound of ack timeout is kMinAckTimeout which is
10ms. This diff changes to use timer's tick interval as lower bound. For the
timer we use today (the default HHWheelTimer), this is a no-op change since the
tick interval is also 10ms.
Reviewed By: mjoras
Differential Revision: D17421600
fbshipit-source-id: 073ac6a8e5d84dbdfc00e8e95ff13be26adb1684
Summary:
Implement sending stream limit updates in a windowed fashion, so that as a peer exhausts its streams we will grant it additional credit. This is implemented by having the stream manager check if an update is needed on removing streams, and the api layer potentially sending an update after it initiates the check for closed streams.
This also makes some driveby changes to use `std::lower_bound` instead of `std::find` for the sorted collections in the stream manager.
Reviewed By: yangchi
Differential Revision: D16808229
fbshipit-source-id: f6e3460d43e4d165e362164be00c0cec27cf1e79
Summary:
we were not setting the MSS.
populate it from the transport, compute the cwnd appropriately and check the netquality header
Reviewed By: yangchi
Differential Revision: D17385975
fbshipit-source-id: 3666728c48d3fc7a65827d526fbdd546449b2e1f
Summary:
This is important in production, but not necessarily for standalone clients.
The transport drain feature is functionally similar to tcp TIME_WAIT, with the
difference that with a userspace transport implementation that forces a client
application to wait for that timeout before it can shutdown
Reviewed By: mjoras
Differential Revision: D17271443
fbshipit-source-id: db5d9e79f175aa86cd9ca8a8a2181b5e80c1d0e8
Summary:
When apps interested in such info retrieve it, it would be bad if we
update the value and mess up the timer drift. So this diff adds a pure getter
function.
Reviewed By: mjoras
Differential Revision: D16918673
fbshipit-source-id: c4954b2f7e3a1d53ec0e6491ef1a25e8f53f1744
Summary:
Use the new Pacer interface in the transport where we currently
directly use CongestinoController interrace for paciner related APIs.
Reviewed By: mjoras
Differential Revision: D16918672
fbshipit-source-id: 410f72d567e71bdd3279e344f6e9abf5e132518e
Summary:
Second try of this diff. This time after the fix of uninitizlied
default constructed std::chrono::duration values in BBR and Copa.
Currently each conegstion controller maintains this value while
TransportSettings also has this value. But currently there is no way they are
in sync. The timer uses the value in TransportSettings. So the value maintained
by each congestion controller isn't used except for tracing, but that also make
tracing wrong.
Reviewed By: oesh
Differential Revision: D16985568
fbshipit-source-id: 5fbd9fee4deec5177b92c733a159e0ba9bfd2289
Summary:
Mostly need a bunch of folly::assume_unreachable() (sometimes
replacing __builtin_unreachable()). One place using designated initializers
that had to get commented out. Some headers replaced with folly portability ones.
Reviewed By: mzlee, phoad
Differential Revision: D16970520
fbshipit-source-id: 1b8a36ecb1975e2dc0869b66c4ea5439baf7575d
Summary:
Currently each conegstion controller maintains this value while
TransportSettings also has this value. But currently there is no way they are
in sync. The timer uses the value in TransportSettings. So the value maintained
by each congestion controller isn't used except for tracing, but that also make
tracing wrong.
Reviewed By: mjoras
Differential Revision: D16772744
fbshipit-source-id: 71800e5dc1f9fce8df4b1f72ce5aa5cfd15fbc89
Summary: Update qlog format to be more complete. Adding the summary section (with extra fields like title, description, etc). This diff is just to make the format more on par with Robin's schema.
Reviewed By: mjoras
Differential Revision: D16499808
fbshipit-source-id: 56cfbb95404f7e3c6638bffda18b53f7d83048a1
Summary: Add transportStateUpdate event so it can be part of qlog.
Reviewed By: mjoras
Differential Revision: D16342467
fbshipit-source-id: 109189275d44996850b82646bab4a733a3a4c7a1
