gfe-relnote: In BandwidthSampler, capture the number of lost bytes at the time a packet is sent, and make it available to caller when the packet is acked or lost. No behavior change, not protected.
In addition to the number of lost bytes, this CL also makes the number of sent bytes and number of acked bytes available to caller when a packet is acked or lost.
This is needed for BBRv2 to set inflight_hi when loss is high.
PiperOrigin-RevId: 242001670
Change-Id: I90543435eb1052b2afb3942d1b744c8153524461
diff --git a/quic/core/congestion_control/bandwidth_sampler.cc b/quic/core/congestion_control/bandwidth_sampler.cc
index c60eaa2..88343b4 100644
--- a/quic/core/congestion_control/bandwidth_sampler.cc
+++ b/quic/core/congestion_control/bandwidth_sampler.cc
@@ -15,6 +15,7 @@
BandwidthSampler::BandwidthSampler()
: total_bytes_sent_(0),
total_bytes_acked_(0),
+ total_bytes_lost_(0),
total_bytes_sent_at_last_acked_packet_(0),
last_acked_packet_sent_time_(QuicTime::Zero()),
last_acked_packet_ack_time_(QuicTime::Zero()),
@@ -87,7 +88,8 @@
QuicPacketNumber packet_number,
const ConnectionStateOnSentPacket& sent_packet) {
total_bytes_acked_ += sent_packet.size;
- total_bytes_sent_at_last_acked_packet_ = sent_packet.total_bytes_sent;
+ total_bytes_sent_at_last_acked_packet_ =
+ sent_packet.send_time_state.total_bytes_sent;
last_acked_packet_sent_time_ = sent_packet.sent_time;
last_acked_packet_ack_time_ = ack_time;
@@ -109,7 +111,7 @@
QuicBandwidth send_rate = QuicBandwidth::Infinite();
if (sent_packet.sent_time > sent_packet.last_acked_packet_sent_time) {
send_rate = QuicBandwidth::FromBytesAndTimeDelta(
- sent_packet.total_bytes_sent -
+ sent_packet.send_time_state.total_bytes_sent -
sent_packet.total_bytes_sent_at_last_acked_packet,
sent_packet.sent_time - sent_packet.last_acked_packet_sent_time);
}
@@ -133,8 +135,7 @@
return BandwidthSample();
}
QuicBandwidth ack_rate = QuicBandwidth::FromBytesAndTimeDelta(
- total_bytes_acked_ -
- sent_packet.total_bytes_acked_at_the_last_acked_packet,
+ total_bytes_acked_ - sent_packet.send_time_state.total_bytes_acked,
ack_time - sent_packet.last_acked_packet_ack_time);
BandwidthSample sample;
@@ -143,17 +144,28 @@
// means that the RTT measurements here can be artificially high, especially
// on low bandwidth connections.
sample.rtt = ack_time - sent_packet.sent_time;
- // A sample is app-limited if the packet was sent during the app-limited
- // phase.
- sample.is_app_limited = sent_packet.is_app_limited;
+ SentPacketToSendTimeState(sent_packet, &sample.state_at_send);
return sample;
}
-void BandwidthSampler::OnPacketLost(QuicPacketNumber packet_number) {
+SendTimeState BandwidthSampler::OnPacketLost(QuicPacketNumber packet_number) {
// TODO(vasilvv): see the comment for the case of missing packets in
// BandwidthSampler::OnPacketAcknowledged on why this does not raise a
// QUIC_BUG when removal fails.
- connection_state_map_.Remove(packet_number);
+ SendTimeState send_time_state;
+ send_time_state.is_valid = connection_state_map_.Remove(
+ packet_number, [&](const ConnectionStateOnSentPacket& sent_packet) {
+ total_bytes_lost_ += sent_packet.size;
+ SentPacketToSendTimeState(sent_packet, &send_time_state);
+ });
+ return send_time_state;
+}
+
+void BandwidthSampler::SentPacketToSendTimeState(
+ const ConnectionStateOnSentPacket& sent_packet,
+ SendTimeState* send_time_state) const {
+ *send_time_state = sent_packet.send_time_state;
+ send_time_state->is_valid = true;
}
void BandwidthSampler::OnAppLimited() {
@@ -162,16 +174,35 @@
}
void BandwidthSampler::RemoveObsoletePackets(QuicPacketNumber least_unacked) {
+ // A packet can become obsolete when it is removed from QuicUnackedPacketMap's
+ // view of inflight before it is acked or marked as lost. For example, when
+ // QuicSentPacketManager::RetransmitCryptoPackets retransmits a crypto packet,
+ // the packet is removed from QuicUnackedPacketMap's inflight, but is not
+ // marked as acked or lost in the BandwidthSampler.
while (!connection_state_map_.IsEmpty() &&
connection_state_map_.first_packet() < least_unacked) {
- connection_state_map_.Remove(connection_state_map_.first_packet());
+ connection_state_map_.Remove(
+ connection_state_map_.first_packet(),
+ [&](const ConnectionStateOnSentPacket& sent_packet) {
+ // Obsoleted packets as either acked or lost but the sampler doesn't
+ // know. We count them as acked here, since most packets are acked.
+ total_bytes_acked_ += sent_packet.size;
+ });
}
}
+QuicByteCount BandwidthSampler::total_bytes_sent() const {
+ return total_bytes_sent_;
+}
+
QuicByteCount BandwidthSampler::total_bytes_acked() const {
return total_bytes_acked_;
}
+QuicByteCount BandwidthSampler::total_bytes_lost() const {
+ return total_bytes_lost_;
+}
+
bool BandwidthSampler::is_app_limited() const {
return is_app_limited_;
}
diff --git a/quic/core/congestion_control/bandwidth_sampler.h b/quic/core/congestion_control/bandwidth_sampler.h
index 69aaae7..4de05b0 100644
--- a/quic/core/congestion_control/bandwidth_sampler.h
+++ b/quic/core/congestion_control/bandwidth_sampler.h
@@ -9,6 +9,7 @@
#include "net/third_party/quiche/src/quic/core/quic_bandwidth.h"
#include "net/third_party/quiche/src/quic/core/quic_packets.h"
#include "net/third_party/quiche/src/quic/core/quic_time.h"
+#include "net/third_party/quiche/src/quic/core/quic_types.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_export.h"
namespace quic {
@@ -17,6 +18,47 @@
class BandwidthSamplerPeer;
} // namespace test
+// A subset of BandwidthSampler::ConnectionStateOnSentPacket which is returned
+// to the caller when the packet is acked or lost.
+struct QUIC_EXPORT_PRIVATE SendTimeState {
+ SendTimeState()
+ : is_valid(false),
+ is_app_limited(false),
+ total_bytes_sent(0),
+ total_bytes_acked(0),
+ total_bytes_lost(0) {}
+
+ SendTimeState(bool is_app_limited,
+ QuicByteCount total_bytes_sent,
+ QuicByteCount total_bytes_acked,
+ QuicByteCount total_bytes_lost)
+ : is_valid(true),
+ is_app_limited(is_app_limited),
+ total_bytes_sent(total_bytes_sent),
+ total_bytes_acked(total_bytes_acked),
+ total_bytes_lost(total_bytes_lost) {}
+
+ SendTimeState(const SendTimeState& other) = default;
+
+ // Whether other states in this object is valid.
+ bool is_valid;
+
+ // Whether the sender is app limited at the time the packet was sent.
+ // App limited bandwidth sample might be artificially low because the sender
+ // did not have enough data to send in order to saturate the link.
+ bool is_app_limited;
+
+ // Total number of sent bytes at the time the packet was sent.
+ // Includes the packet itself.
+ QuicByteCount total_bytes_sent;
+
+ // Total number of acked bytes at the time the packet was sent.
+ QuicByteCount total_bytes_acked;
+
+ // Total number of lost bytes at the time the packet was sent.
+ QuicByteCount total_bytes_lost;
+};
+
struct QUIC_EXPORT_PRIVATE BandwidthSample {
// The bandwidth at that particular sample. Zero if no valid bandwidth sample
// is available.
@@ -26,14 +68,11 @@
// available. Does not correct for delayed ack time.
QuicTime::Delta rtt;
- // Indicates whether the sample might be artificially low because the sender
- // did not have enough data to send in order to saturate the link.
- bool is_app_limited;
+ // States captured when the packet was sent.
+ SendTimeState state_at_send;
BandwidthSample()
- : bandwidth(QuicBandwidth::Zero()),
- rtt(QuicTime::Delta::Zero()),
- is_app_limited(false) {}
+ : bandwidth(QuicBandwidth::Zero()), rtt(QuicTime::Delta::Zero()) {}
};
// An interface common to any class that can provide bandwidth samples from the
@@ -62,7 +101,7 @@
// Informs the sampler that a packet is considered lost and it should no
// longer keep track of it.
- virtual void OnPacketLost(QuicPacketNumber packet_number) = 0;
+ virtual SendTimeState OnPacketLost(QuicPacketNumber packet_number) = 0;
// Informs the sampler that the connection is currently app-limited, causing
// the sampler to enter the app-limited phase. The phase will expire by
@@ -72,11 +111,14 @@
// Remove all the packets lower than the specified packet number.
virtual void RemoveObsoletePackets(QuicPacketNumber least_unacked) = 0;
- // Total number of bytes currently acknowledged by the receiver.
+ // Total number of bytes sent/acked/lost in the connection.
+ virtual QuicByteCount total_bytes_sent() const = 0;
virtual QuicByteCount total_bytes_acked() const = 0;
+ virtual QuicByteCount total_bytes_lost() const = 0;
// Application-limited information exported for debugging.
virtual bool is_app_limited() const = 0;
+
virtual QuicPacketNumber end_of_app_limited_phase() const = 0;
};
@@ -172,14 +214,18 @@
HasRetransmittableData has_retransmittable_data) override;
BandwidthSample OnPacketAcknowledged(QuicTime ack_time,
QuicPacketNumber packet_number) override;
- void OnPacketLost(QuicPacketNumber packet_number) override;
+ SendTimeState OnPacketLost(QuicPacketNumber packet_number) override;
void OnAppLimited() override;
void RemoveObsoletePackets(QuicPacketNumber least_unacked) override;
+ QuicByteCount total_bytes_sent() const override;
QuicByteCount total_bytes_acked() const override;
+ QuicByteCount total_bytes_lost() const override;
+
bool is_app_limited() const override;
+
QuicPacketNumber end_of_app_limited_phase() const override;
private:
@@ -196,10 +242,6 @@
// Size of the packet.
QuicByteCount size;
- // The value of |total_bytes_sent_| at the time the packet was sent.
- // Includes the packet itself.
- QuicByteCount total_bytes_sent;
-
// The value of |total_bytes_sent_at_last_acked_packet_| at the time the
// packet was sent.
QuicByteCount total_bytes_sent_at_last_acked_packet;
@@ -212,13 +254,9 @@
// sent.
QuicTime last_acked_packet_ack_time;
- // The value of |total_bytes_acked_| at the time the packet was
- // sent.
- QuicByteCount total_bytes_acked_at_the_last_acked_packet;
-
- // The value of |is_app_limited_| at the time the packet was
- // sent.
- bool is_app_limited;
+ // Send time states that are returned to the congestion controller when the
+ // packet is acked or lost.
+ SendTimeState send_time_state;
// Snapshot constructor. Records the current state of the bandwidth
// sampler.
@@ -227,34 +265,39 @@
const BandwidthSampler& sampler)
: sent_time(sent_time),
size(size),
- total_bytes_sent(sampler.total_bytes_sent_),
total_bytes_sent_at_last_acked_packet(
sampler.total_bytes_sent_at_last_acked_packet_),
last_acked_packet_sent_time(sampler.last_acked_packet_sent_time_),
last_acked_packet_ack_time(sampler.last_acked_packet_ack_time_),
- total_bytes_acked_at_the_last_acked_packet(
- sampler.total_bytes_acked_),
- is_app_limited(sampler.is_app_limited_) {}
+ send_time_state(sampler.is_app_limited_,
+ sampler.total_bytes_sent_,
+ sampler.total_bytes_acked_,
+ sampler.total_bytes_lost_) {}
// Default constructor. Required to put this structure into
// PacketNumberIndexedQueue.
ConnectionStateOnSentPacket()
: sent_time(QuicTime::Zero()),
size(0),
- total_bytes_sent(0),
total_bytes_sent_at_last_acked_packet(0),
last_acked_packet_sent_time(QuicTime::Zero()),
- last_acked_packet_ack_time(QuicTime::Zero()),
- total_bytes_acked_at_the_last_acked_packet(0),
- is_app_limited(false) {}
+ last_acked_packet_ack_time(QuicTime::Zero()) {}
};
+ // Copy a subset of the (private) ConnectionStateOnSentPacket to the (public)
+ // SendTimeState. Always set send_time_state->is_valid to true.
+ void SentPacketToSendTimeState(const ConnectionStateOnSentPacket& sent_packet,
+ SendTimeState* send_time_state) const;
+
// The total number of congestion controlled bytes sent during the connection.
QuicByteCount total_bytes_sent_;
// The total number of congestion controlled bytes which were acknowledged.
QuicByteCount total_bytes_acked_;
+ // The total number of congestion controlled bytes which were lost.
+ QuicByteCount total_bytes_lost_;
+
// The value of |total_bytes_sent_| at the time the last acknowledged packet
// was sent. Valid only when |last_acked_packet_sent_time_| is valid.
QuicByteCount total_bytes_sent_at_last_acked_packet_;
diff --git a/quic/core/congestion_control/bandwidth_sampler_test.cc b/quic/core/congestion_control/bandwidth_sampler_test.cc
index e9b74c7..077e9a9 100644
--- a/quic/core/congestion_control/bandwidth_sampler_test.cc
+++ b/quic/core/congestion_control/bandwidth_sampler_test.cc
@@ -4,6 +4,7 @@
#include "net/third_party/quiche/src/quic/core/congestion_control/bandwidth_sampler.h"
+#include "net/third_party/quiche/src/quic/core/quic_types.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_flags.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_test.h"
#include "net/third_party/quiche/src/quic/test_tools/mock_clock.h"
@@ -40,6 +41,10 @@
BandwidthSampler sampler_;
QuicByteCount bytes_in_flight_;
+ QuicByteCount PacketsToBytes(QuicPacketCount packet_count) {
+ return packet_count * kRegularPacketSize;
+ }
+
void SendPacketInner(uint64_t packet_number,
QuicByteCount bytes,
HasRetransmittableData has_retransmittable_data) {
@@ -66,15 +71,19 @@
// Acknowledge receipt of a packet and expect it to be not app-limited.
QuicBandwidth AckPacket(uint64_t packet_number) {
BandwidthSample sample = AckPacketInner(packet_number);
- EXPECT_FALSE(sample.is_app_limited);
+ EXPECT_TRUE(sample.state_at_send.is_valid);
+ EXPECT_FALSE(sample.state_at_send.is_app_limited);
return sample.bandwidth;
}
- void LosePacket(uint64_t packet_number) {
+ SendTimeState LosePacket(uint64_t packet_number) {
QuicByteCount size = BandwidthSamplerPeer::GetPacketSize(
sampler_, QuicPacketNumber(packet_number));
bytes_in_flight_ -= size;
- sampler_.OnPacketLost(QuicPacketNumber(packet_number));
+ SendTimeState send_time_state =
+ sampler_.OnPacketLost(QuicPacketNumber(packet_number));
+ EXPECT_TRUE(send_time_state.is_valid);
+ return send_time_state;
}
// Sends one packet and acks it. Then, send 20 packets. Finally, send
@@ -124,6 +133,63 @@
EXPECT_EQ(0u, bytes_in_flight_);
}
+TEST_F(BandwidthSamplerTest, SendTimeState) {
+ QuicTime::Delta time_between_packets = QuicTime::Delta::FromMilliseconds(10);
+
+ // Send packets 1-5.
+ for (int i = 1; i <= 5; i++) {
+ SendPacket(i);
+ EXPECT_EQ(PacketsToBytes(i), sampler_.total_bytes_sent());
+ clock_.AdvanceTime(time_between_packets);
+ }
+
+ // Ack packet 1.
+ SendTimeState send_time_state = AckPacketInner(1).state_at_send;
+ EXPECT_EQ(PacketsToBytes(1), send_time_state.total_bytes_sent);
+ EXPECT_EQ(0, send_time_state.total_bytes_acked);
+ EXPECT_EQ(0, send_time_state.total_bytes_lost);
+ EXPECT_EQ(PacketsToBytes(1), sampler_.total_bytes_acked());
+
+ // Lose packet 2.
+ send_time_state = LosePacket(2);
+ EXPECT_EQ(PacketsToBytes(2), send_time_state.total_bytes_sent);
+ EXPECT_EQ(0, send_time_state.total_bytes_acked);
+ EXPECT_EQ(0, send_time_state.total_bytes_lost);
+ EXPECT_EQ(PacketsToBytes(1), sampler_.total_bytes_lost());
+
+ // Lose packet 3.
+ send_time_state = LosePacket(3);
+ EXPECT_EQ(PacketsToBytes(3), send_time_state.total_bytes_sent);
+ EXPECT_EQ(0, send_time_state.total_bytes_acked);
+ EXPECT_EQ(0, send_time_state.total_bytes_lost);
+ EXPECT_EQ(PacketsToBytes(2), sampler_.total_bytes_lost());
+
+ // Send packets 6-10.
+ for (int i = 6; i <= 10; i++) {
+ SendPacket(i);
+ EXPECT_EQ(PacketsToBytes(i), sampler_.total_bytes_sent());
+ clock_.AdvanceTime(time_between_packets);
+ }
+
+ // Ack all inflight packets.
+ QuicPacketCount acked_packet_count = 1;
+ EXPECT_EQ(PacketsToBytes(acked_packet_count), sampler_.total_bytes_acked());
+ for (int i = 4; i <= 10; i++) {
+ send_time_state = AckPacketInner(i).state_at_send;
+ ++acked_packet_count;
+ EXPECT_EQ(PacketsToBytes(acked_packet_count), sampler_.total_bytes_acked());
+ EXPECT_EQ(PacketsToBytes(i), send_time_state.total_bytes_sent);
+ if (i <= 5) {
+ EXPECT_EQ(0, send_time_state.total_bytes_acked);
+ EXPECT_EQ(0, send_time_state.total_bytes_lost);
+ } else {
+ EXPECT_EQ(PacketsToBytes(1), send_time_state.total_bytes_acked);
+ EXPECT_EQ(PacketsToBytes(2), send_time_state.total_bytes_lost);
+ }
+ clock_.AdvanceTime(time_between_packets);
+ }
+}
+
// Test the sampler during regular windowed sender scenario with fixed
// CWND of 20.
TEST_F(BandwidthSamplerTest, SendPaced) {
@@ -321,7 +387,7 @@
// app-limited and underestimate the bandwidth due to that.
for (int i = 41; i <= 60; i++) {
BandwidthSample sample = AckPacketInner(i);
- EXPECT_TRUE(sample.is_app_limited);
+ EXPECT_TRUE(sample.state_at_send.is_app_limited);
EXPECT_LT(sample.bandwidth, 0.7f * expected_bandwidth);
SendPacket(i + 20);
diff --git a/quic/core/congestion_control/bbr_sender.cc b/quic/core/congestion_control/bbr_sender.cc
index e59aa8d..5c9e9ea 100644
--- a/quic/core/congestion_control/bbr_sender.cc
+++ b/quic/core/congestion_control/bbr_sender.cc
@@ -505,13 +505,14 @@
}
BandwidthSample bandwidth_sample =
sampler_.OnPacketAcknowledged(now, packet.packet_number);
- last_sample_is_app_limited_ = bandwidth_sample.is_app_limited;
- has_non_app_limited_sample_ |= !bandwidth_sample.is_app_limited;
+ last_sample_is_app_limited_ = bandwidth_sample.state_at_send.is_app_limited;
+ has_non_app_limited_sample_ |=
+ !bandwidth_sample.state_at_send.is_app_limited;
if (!bandwidth_sample.rtt.IsZero()) {
sample_min_rtt = std::min(sample_min_rtt, bandwidth_sample.rtt);
}
- if (!bandwidth_sample.is_app_limited ||
+ if (!bandwidth_sample.state_at_send.is_app_limited ||
bandwidth_sample.bandwidth > BandwidthEstimate()) {
max_bandwidth_.Update(bandwidth_sample.bandwidth, round_trip_count_);
}
diff --git a/quic/core/packet_number_indexed_queue.h b/quic/core/packet_number_indexed_queue.h
index ab2e20b..695ff7d 100644
--- a/quic/core/packet_number_indexed_queue.h
+++ b/quic/core/packet_number_indexed_queue.h
@@ -6,6 +6,7 @@
#define QUICHE_QUIC_CORE_PACKET_NUMBER_INDEXED_QUEUE_H_
#include "net/third_party/quiche/src/quic/core/quic_constants.h"
+#include "net/third_party/quiche/src/quic/core/quic_packet_number.h"
#include "net/third_party/quiche/src/quic/core/quic_types.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_bug_tracker.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_containers.h"
@@ -54,6 +55,11 @@
// queue as necessary.
bool Remove(QuicPacketNumber packet_number);
+ // Same as above, but if an entry is present in the queue, also call f(entry)
+ // before removing it.
+ template <typename Function>
+ bool Remove(QuicPacketNumber packet_number, Function f);
+
bool IsEmpty() const { return number_of_present_entries_ == 0; }
// Returns the number of entries in the queue.
@@ -161,10 +167,18 @@
template <typename T>
bool PacketNumberIndexedQueue<T>::Remove(QuicPacketNumber packet_number) {
+ return Remove(packet_number, [](const T&) {});
+}
+
+template <typename T>
+template <typename Function>
+bool PacketNumberIndexedQueue<T>::Remove(QuicPacketNumber packet_number,
+ Function f) {
EntryWrapper* entry = GetEntryWrapper(packet_number);
if (entry == nullptr) {
return false;
}
+ f(*static_cast<const T*>(entry));
entry->present = false;
number_of_present_entries_--;
