gfe-relnote: Refactor how QuicConnectionStats.slowstart_duration is calculated in QUIC BBR1, and start populate it in QUIC BBR2. Not protected. (Worst case we get some incorrect values in transport connection stats)
Similar to slowstart_duration, I will add drain_duration, probe_rtt_duration, probe_bw_(down|up|cruise)_duration, in a follow up CL.
PiperOrigin-RevId: 282224334
Change-Id: I873c39dcd47eb03d22d245cdfbf13aabc1a814e6
diff --git a/quic/core/congestion_control/bbr2_drain.h b/quic/core/congestion_control/bbr2_drain.h
index ccc0320..e083aeb 100644
--- a/quic/core/congestion_control/bbr2_drain.h
+++ b/quic/core/congestion_control/bbr2_drain.h
@@ -18,6 +18,7 @@
using Bbr2ModeBase::Bbr2ModeBase;
void Enter(const Bbr2CongestionEvent& congestion_event) override;
+ void Leave(const Bbr2CongestionEvent& /*congestion_event*/) override {}
Bbr2Mode OnCongestionEvent(
QuicByteCount prior_in_flight,
diff --git a/quic/core/congestion_control/bbr2_misc.h b/quic/core/congestion_control/bbr2_misc.h
index 3e8a518..139b120 100644
--- a/quic/core/congestion_control/bbr2_misc.h
+++ b/quic/core/congestion_control/bbr2_misc.h
@@ -473,7 +473,9 @@
virtual ~Bbr2ModeBase() = default;
+ // Called when entering/leaving this mode.
virtual void Enter(const Bbr2CongestionEvent& congestion_event) = 0;
+ virtual void Leave(const Bbr2CongestionEvent& congestion_event) = 0;
virtual Bbr2Mode OnCongestionEvent(
QuicByteCount prior_in_flight,
diff --git a/quic/core/congestion_control/bbr2_probe_bw.h b/quic/core/congestion_control/bbr2_probe_bw.h
index 429b6fe..d64bd65 100644
--- a/quic/core/congestion_control/bbr2_probe_bw.h
+++ b/quic/core/congestion_control/bbr2_probe_bw.h
@@ -20,6 +20,7 @@
using Bbr2ModeBase::Bbr2ModeBase;
void Enter(const Bbr2CongestionEvent& congestion_event) override;
+ void Leave(const Bbr2CongestionEvent& /*congestion_event*/) override {}
Bbr2Mode OnCongestionEvent(
QuicByteCount prior_in_flight,
diff --git a/quic/core/congestion_control/bbr2_probe_rtt.h b/quic/core/congestion_control/bbr2_probe_rtt.h
index c112a33..80a9d93 100644
--- a/quic/core/congestion_control/bbr2_probe_rtt.h
+++ b/quic/core/congestion_control/bbr2_probe_rtt.h
@@ -18,6 +18,7 @@
using Bbr2ModeBase::Bbr2ModeBase;
void Enter(const Bbr2CongestionEvent& congestion_event) override;
+ void Leave(const Bbr2CongestionEvent& /*congestion_event*/) override {}
Bbr2Mode OnCongestionEvent(
QuicByteCount prior_in_flight,
diff --git a/quic/core/congestion_control/bbr2_sender.cc b/quic/core/congestion_control/bbr2_sender.cc
index c90decd..73787a2 100644
--- a/quic/core/congestion_control/bbr2_sender.cc
+++ b/quic/core/congestion_control/bbr2_sender.cc
@@ -57,11 +57,12 @@
QuicPacketCount initial_cwnd_in_packets,
QuicPacketCount max_cwnd_in_packets,
QuicRandom* random,
- QuicConnectionStats* /*stats*/)
+ QuicConnectionStats* stats)
: mode_(Bbr2Mode::STARTUP),
rtt_stats_(rtt_stats),
unacked_packets_(unacked_packets),
random_(random),
+ connection_stats_(stats),
params_(kDefaultMinimumCongestionWindow,
max_cwnd_in_packets * kDefaultTCPMSS),
model_(¶ms_,
@@ -75,7 +76,7 @@
pacing_rate_(kInitialPacingGain * QuicBandwidth::FromBytesAndTimeDelta(
cwnd_,
rtt_stats->SmoothedOrInitialRtt())),
- startup_(this, &model_),
+ startup_(this, &model_, now),
drain_(this, &model_),
probe_bw_(this, &model_),
probe_rtt_(this, &model_),
@@ -168,6 +169,7 @@
QUIC_DVLOG(2) << this << " Mode change: " << mode_ << " ==> " << next_mode
<< " @ " << event_time;
+ BBR2_MODE_DISPATCH(Leave(congestion_event));
mode_ = next_mode;
BBR2_MODE_DISPATCH(Enter(congestion_event));
--mode_changes_allowed;
diff --git a/quic/core/congestion_control/bbr2_sender.h b/quic/core/congestion_control/bbr2_sender.h
index bff38f6..f0a3b5f 100644
--- a/quic/core/congestion_control/bbr2_sender.h
+++ b/quic/core/congestion_control/bbr2_sender.h
@@ -30,7 +30,7 @@
QuicPacketCount initial_cwnd_in_packets,
QuicPacketCount max_cwnd_in_packets,
QuicRandom* random,
- QuicConnectionStats* /*stats*/);
+ QuicConnectionStats* stats);
~Bbr2Sender() override = default;
@@ -157,6 +157,7 @@
const RttStats* const rtt_stats_;
const QuicUnackedPacketMap* const unacked_packets_;
QuicRandom* random_;
+ QuicConnectionStats* connection_stats_;
const Bbr2Params params_;
diff --git a/quic/core/congestion_control/bbr2_simulator_test.cc b/quic/core/congestion_control/bbr2_simulator_test.cc
index cab9cb8..005e11d 100644
--- a/quic/core/congestion_control/bbr2_simulator_test.cc
+++ b/quic/core/congestion_control/bbr2_simulator_test.cc
@@ -24,6 +24,10 @@
#include "net/third_party/quiche/src/quic/test_tools/simulator/switch.h"
#include "net/third_party/quiche/src/quic/test_tools/simulator/traffic_policer.h"
+using testing::AllOf;
+using testing::Ge;
+using testing::Le;
+
namespace quic {
using CyclePhase = Bbr2ProbeBwMode::CyclePhase;
@@ -676,6 +680,25 @@
EXPECT_LE(sender_loss_rate_in_packets(), 0.30);
}
+// TODO(wub): Add other slowstart stats to BBRv2.
+TEST_F(Bbr2DefaultTopologyTest, StartupStats) {
+ DefaultTopologyParams params;
+ CreateNetwork(params);
+
+ DriveOutOfStartup(params);
+ ASSERT_FALSE(sender_->InSlowStart());
+
+ const QuicConnectionStats& stats = sender_connection_stats();
+ EXPECT_EQ(1u, stats.slowstart_count);
+ EXPECT_FALSE(stats.slowstart_duration.IsRunning());
+ EXPECT_THAT(stats.slowstart_duration.GetTotalElapsedTime(),
+ AllOf(Ge(QuicTime::Delta::FromMilliseconds(500)),
+ Le(QuicTime::Delta::FromMilliseconds(1500))));
+ EXPECT_EQ(stats.slowstart_duration.GetTotalElapsedTime(),
+ QuicConnectionPeer::GetSentPacketManager(sender_connection())
+ ->GetSlowStartDuration());
+}
+
// All Bbr2MultiSenderTests uses the following network topology:
//
// Sender 0 (A Bbr2Sender)
diff --git a/quic/core/congestion_control/bbr2_startup.cc b/quic/core/congestion_control/bbr2_startup.cc
index 5ff1050..187b349 100644
--- a/quic/core/congestion_control/bbr2_startup.cc
+++ b/quic/core/congestion_control/bbr2_startup.cc
@@ -12,17 +12,30 @@
namespace quic {
Bbr2StartupMode::Bbr2StartupMode(const Bbr2Sender* sender,
- Bbr2NetworkModel* model)
+ Bbr2NetworkModel* model,
+ QuicTime now)
: Bbr2ModeBase(sender, model),
full_bandwidth_reached_(false),
full_bandwidth_baseline_(QuicBandwidth::Zero()),
rounds_without_bandwidth_growth_(0),
- loss_events_in_round_(0) {}
+ loss_events_in_round_(0) {
+ // Clear some startup stats if |sender_->connection_stats_| has been used by
+ // another sender, which happens e.g. when QuicConnection switch send
+ // algorithms.
+ sender_->connection_stats_->slowstart_count = 1;
+ sender_->connection_stats_->slowstart_duration = QuicTimeAccumulator();
+ sender_->connection_stats_->slowstart_duration.Start(now);
+}
void Bbr2StartupMode::Enter(const Bbr2CongestionEvent& /*congestion_event*/) {
QUIC_BUG << "Bbr2StartupMode::Enter should not be called";
}
+void Bbr2StartupMode::Leave(const Bbr2CongestionEvent& congestion_event) {
+ sender_->connection_stats_->slowstart_duration.Stop(
+ congestion_event.event_time);
+}
+
Bbr2Mode Bbr2StartupMode::OnCongestionEvent(
QuicByteCount /*prior_in_flight*/,
QuicTime /*event_time*/,
diff --git a/quic/core/congestion_control/bbr2_startup.h b/quic/core/congestion_control/bbr2_startup.h
index 489bd55..80539d9 100644
--- a/quic/core/congestion_control/bbr2_startup.h
+++ b/quic/core/congestion_control/bbr2_startup.h
@@ -16,9 +16,12 @@
class Bbr2Sender;
class QUIC_EXPORT_PRIVATE Bbr2StartupMode final : public Bbr2ModeBase {
public:
- Bbr2StartupMode(const Bbr2Sender* sender, Bbr2NetworkModel* model);
+ Bbr2StartupMode(const Bbr2Sender* sender,
+ Bbr2NetworkModel* model,
+ QuicTime now);
void Enter(const Bbr2CongestionEvent& congestion_event) override;
+ void Leave(const Bbr2CongestionEvent& congestion_event) override;
Bbr2Mode OnCongestionEvent(
QuicByteCount prior_in_flight,
diff --git a/quic/core/congestion_control/bbr_sender.cc b/quic/core/congestion_control/bbr_sender.cc
index 93ee5cb..4217048 100644
--- a/quic/core/congestion_control/bbr_sender.cc
+++ b/quic/core/congestion_control/bbr_sender.cc
@@ -11,6 +11,7 @@
#include "net/third_party/quiche/src/quic/core/congestion_control/rtt_stats.h"
#include "net/third_party/quiche/src/quic/core/crypto/crypto_protocol.h"
#include "net/third_party/quiche/src/quic/core/quic_time.h"
+#include "net/third_party/quiche/src/quic/core/quic_time_accumulator.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_bug_tracker.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_fallthrough.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_flag_utils.h"
@@ -136,8 +137,10 @@
app_limited_since_last_probe_rtt_(false),
min_rtt_since_last_probe_rtt_(QuicTime::Delta::Infinite()) {
if (stats_) {
+ // Clear some startup stats if |stats_| has been used by another sender,
+ // which happens e.g. when QuicConnection switch send algorithms.
stats_->slowstart_count = 0;
- stats_->slowstart_start_time = QuicTime::Zero();
+ stats_->slowstart_duration = QuicTimeAccumulator();
}
EnterStartupMode(now);
}
@@ -465,8 +468,7 @@
void BbrSender::EnterStartupMode(QuicTime now) {
if (stats_) {
++stats_->slowstart_count;
- DCHECK_EQ(stats_->slowstart_start_time, QuicTime::Zero()) << mode_;
- stats_->slowstart_start_time = now;
+ stats_->slowstart_duration.Start(now);
}
mode_ = STARTUP;
pacing_gain_ = high_gain_;
@@ -671,12 +673,7 @@
void BbrSender::OnExitStartup(QuicTime now) {
DCHECK_EQ(mode_, STARTUP);
if (stats_) {
- DCHECK_NE(stats_->slowstart_start_time, QuicTime::Zero());
- if (now > stats_->slowstart_start_time) {
- stats_->slowstart_duration =
- now - stats_->slowstart_start_time + stats_->slowstart_duration;
- }
- stats_->slowstart_start_time = QuicTime::Zero();
+ stats_->slowstart_duration.Stop(now);
}
}
diff --git a/quic/core/congestion_control/bbr_sender_test.cc b/quic/core/congestion_control/bbr_sender_test.cc
index 60b303e..6acfe80 100644
--- a/quic/core/congestion_control/bbr_sender_test.cc
+++ b/quic/core/congestion_control/bbr_sender_test.cc
@@ -1309,11 +1309,11 @@
EXPECT_THAT(stats.slowstart_bytes_sent, AllOf(Ge(100000u), Le(1000000u)));
EXPECT_LE(stats.slowstart_packets_lost, 10u);
EXPECT_LE(stats.slowstart_bytes_lost, 10000u);
- EXPECT_THAT(stats.slowstart_duration,
+ EXPECT_FALSE(stats.slowstart_duration.IsRunning());
+ EXPECT_THAT(stats.slowstart_duration.GetTotalElapsedTime(),
AllOf(Ge(QuicTime::Delta::FromMilliseconds(500)),
Le(QuicTime::Delta::FromMilliseconds(1500))));
- EXPECT_EQ(QuicTime::Zero(), stats.slowstart_start_time);
- EXPECT_EQ(stats.slowstart_duration,
+ EXPECT_EQ(stats.slowstart_duration.GetTotalElapsedTime(),
QuicConnectionPeer::GetSentPacketManager(bbr_sender_.connection())
->GetSlowStartDuration());
}
