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());
}
diff --git a/quic/core/quic_connection_stats.cc b/quic/core/quic_connection_stats.cc
index e3652b0..ff067bd 100644
--- a/quic/core/quic_connection_stats.cc
+++ b/quic/core/quic_connection_stats.cc
@@ -26,8 +26,6 @@
slowstart_bytes_sent(0),
slowstart_packets_lost(0),
slowstart_bytes_lost(0),
- slowstart_duration(QuicTime::Delta::Zero()),
- slowstart_start_time(QuicTime::Zero()),
packets_dropped(0),
undecryptable_packets_received_before_handshake_complete(0),
crypto_retransmit_count(0),
diff --git a/quic/core/quic_connection_stats.h b/quic/core/quic_connection_stats.h
index a67d867..552e8a6 100644
--- a/quic/core/quic_connection_stats.h
+++ b/quic/core/quic_connection_stats.h
@@ -11,6 +11,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_time_accumulator.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_export.h"
namespace quic {
@@ -60,10 +61,8 @@
QuicPacketCount slowstart_packets_lost;
// Number of bytes lost exiting slow start.
QuicByteCount slowstart_bytes_lost;
- // Time spent in COMPLETED slow start phases.
- QuicTime::Delta slowstart_duration;
- // Start time of the last slow start phase.
- QuicTime slowstart_start_time;
+ // Time spent in slow start. Populated for BBRv1 and BBRv2.
+ QuicTimeAccumulator slowstart_duration;
QuicPacketCount packets_dropped; // Duplicate or less than least unacked.
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc
index ba3c081..bd0343b 100644
--- a/quic/core/quic_sent_packet_manager.cc
+++ b/quic/core/quic_sent_packet_manager.cc
@@ -1077,16 +1077,12 @@
}
QuicTime::Delta QuicSentPacketManager::GetSlowStartDuration() const {
- if (send_algorithm_->GetCongestionControlType() != kBBR) {
- return QuicTime::Delta::Infinite();
+ if (send_algorithm_->GetCongestionControlType() == kBBR ||
+ send_algorithm_->GetCongestionControlType() == kBBRv2) {
+ return stats_->slowstart_duration.GetTotalElapsedTime(
+ clock_->ApproximateNow());
}
-
- if (!send_algorithm_->InSlowStart()) {
- return stats_->slowstart_duration;
- }
-
- return clock_->ApproximateNow() - stats_->slowstart_start_time +
- stats_->slowstart_duration;
+ return QuicTime::Delta::Infinite();
}
std::string QuicSentPacketManager::GetDebugState() const {
diff --git a/quic/core/quic_time_accumulator.h b/quic/core/quic_time_accumulator.h
new file mode 100644
index 0000000..772e5ff
--- /dev/null
+++ b/quic/core/quic_time_accumulator.h
@@ -0,0 +1,66 @@
+// Copyright (c) 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef QUICHE_QUIC_CORE_QUIC_TIME_ACCUMULATOR_H_
+#define QUICHE_QUIC_CORE_QUIC_TIME_ACCUMULATOR_H_
+
+#include "net/third_party/quiche/src/quic/core/quic_time.h"
+#include "net/third_party/quiche/src/quic/platform/api/quic_export.h"
+#include "net/third_party/quiche/src/quic/platform/api/quic_logging.h"
+
+namespace quic {
+
+// QuicTimeAccumulator accumulates elapsed times between Start(s) and Stop(s).
+class QUIC_EXPORT_PRIVATE QuicTimeAccumulator {
+ static constexpr QuicTime kNotRunningSentinel = QuicTime::Infinite();
+
+ public:
+ // True if Started and not Stopped.
+ bool IsRunning() const { return last_start_time_ != kNotRunningSentinel; }
+
+ void Start(QuicTime now) {
+ DCHECK(!IsRunning());
+ last_start_time_ = now;
+ DCHECK(IsRunning());
+ }
+
+ void Stop(QuicTime now) {
+ DCHECK(IsRunning());
+ if (now > last_start_time_) {
+ total_elapsed_ = total_elapsed_ + (now - last_start_time_);
+ }
+ last_start_time_ = kNotRunningSentinel;
+ DCHECK(!IsRunning());
+ }
+
+ // Get total elapsed time between COMPLETED Start/Stop pairs.
+ QuicTime::Delta GetTotalElapsedTime() const { return total_elapsed_; }
+
+ // Get total elapsed time between COMPLETED Start/Stop pairs, plus, if it is
+ // running, the elapsed time between |last_start_time_| and |now|.
+ QuicTime::Delta GetTotalElapsedTime(QuicTime now) const {
+ if (!IsRunning()) {
+ return total_elapsed_;
+ }
+ if (now <= last_start_time_) {
+ return total_elapsed_;
+ }
+ return total_elapsed_ + (now - last_start_time_);
+ }
+
+ private:
+ //
+ // |last_start_time_|
+ // |
+ // V
+ // Start => Stop => Start => Stop => Start
+ // | | | |
+ // |___________| + |___________| = |total_elapsed_|
+ QuicTime::Delta total_elapsed_ = QuicTime::Delta::Zero();
+ QuicTime last_start_time_ = kNotRunningSentinel;
+};
+
+} // namespace quic
+
+#endif // QUICHE_QUIC_CORE_QUIC_TIME_ACCUMULATOR_H_
diff --git a/quic/core/quic_time_accumulator_test.cc b/quic/core/quic_time_accumulator_test.cc
new file mode 100644
index 0000000..e7bc43a
--- /dev/null
+++ b/quic/core/quic_time_accumulator_test.cc
@@ -0,0 +1,82 @@
+// Copyright (c) 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/third_party/quiche/src/quic/core/quic_time_accumulator.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"
+
+namespace quic {
+namespace test {
+
+TEST(QuicTimeAccumulator, DefaultConstruct) {
+ MockClock clock;
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(1));
+
+ QuicTimeAccumulator acc;
+ EXPECT_FALSE(acc.IsRunning());
+
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(1));
+ EXPECT_EQ(QuicTime::Delta::Zero(), acc.GetTotalElapsedTime());
+ EXPECT_EQ(QuicTime::Delta::Zero(), acc.GetTotalElapsedTime(clock.Now()));
+}
+
+TEST(QuicTimeAccumulator, StartStop) {
+ MockClock clock;
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(1));
+
+ QuicTimeAccumulator acc;
+ acc.Start(clock.Now());
+ EXPECT_TRUE(acc.IsRunning());
+
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(10));
+ acc.Stop(clock.Now());
+ EXPECT_FALSE(acc.IsRunning());
+
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(5));
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(10), acc.GetTotalElapsedTime());
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(10),
+ acc.GetTotalElapsedTime(clock.Now()));
+
+ acc.Start(clock.Now());
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(5));
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(10), acc.GetTotalElapsedTime());
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(15),
+ acc.GetTotalElapsedTime(clock.Now()));
+
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(5));
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(10), acc.GetTotalElapsedTime());
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(20),
+ acc.GetTotalElapsedTime(clock.Now()));
+
+ acc.Stop(clock.Now());
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(20), acc.GetTotalElapsedTime());
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(20),
+ acc.GetTotalElapsedTime(clock.Now()));
+}
+
+TEST(QuicTimeAccumulator, ClockStepBackwards) {
+ MockClock clock;
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(100));
+
+ QuicTimeAccumulator acc;
+ acc.Start(clock.Now());
+
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(-10));
+ acc.Stop(clock.Now());
+ EXPECT_EQ(QuicTime::Delta::Zero(), acc.GetTotalElapsedTime());
+ EXPECT_EQ(QuicTime::Delta::Zero(), acc.GetTotalElapsedTime(clock.Now()));
+
+ acc.Start(clock.Now());
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(50));
+ acc.Stop(clock.Now());
+
+ acc.Start(clock.Now());
+ clock.AdvanceTime(QuicTime::Delta::FromMilliseconds(-80));
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(50), acc.GetTotalElapsedTime());
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(50),
+ acc.GetTotalElapsedTime(clock.Now()));
+}
+
+} // namespace test
+} // namespace quic
