Skip to content

Commit

Permalink
Revert "fix: Path is not app limited if more data is available to tra…
Browse files Browse the repository at this point in the history 
…nsmit (#1326)"

This reverts commit eb879b8.
  • Loading branch information
@camshaft
camshaft committed Jun 3, 2022
1 parent 6d41761 commit 91bd325
Show file tree
Hide file tree
Showing 16 changed files with 101 additions and 325 deletions.
21 changes: 2 additions & 19 deletions quic/s2n-quic-core/src/recovery/bandwidth/estimator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,6 @@ pub struct Estimator {
/// The send time of the packet that was most recently marked as delivered, or if the connection
/// was recently idle, the send time of the first packet sent after resuming from idle.
first_sent_time: Option<Timestamp>,
/// The `delivered_bytes` that marks the end of the current application-limited period, or
/// `None` if the connection is not currently application-limited.
app_limited_delivered_bytes: Option<u64>,
rate_sample: RateSample,
}

Expand All @@ -161,7 +158,7 @@ impl Estimator {
pub fn on_packet_sent(
&mut self,
bytes_in_flight: u32,
app_limited: Option<bool>,
is_app_limited: bool,
now: Timestamp,
) -> PacketInfo {
//= https://tools.ietf.org/id/draft-cheng-iccrg-delivery-rate-estimation-02#3.2
Expand All @@ -174,10 +171,6 @@ impl Estimator {
self.delivered_time = Some(now);
}

if app_limited.unwrap_or(false) {
self.app_limited_delivered_bytes = Some(self.delivered_bytes + bytes_in_flight as u64);
}

PacketInfo {
delivered_bytes: self.delivered_bytes,
delivered_time: self
Expand All @@ -188,7 +181,7 @@ impl Estimator {
.first_sent_time
.expect("initialized on first sent packet"),
bytes_in_flight,
is_app_limited: self.app_limited_delivered_bytes.is_some(),
is_app_limited,
}
}

Expand All @@ -207,16 +200,6 @@ impl Estimator {
self.delivered_bytes += bytes_acknowledged as u64;
self.delivered_time = Some(now);

if self
.app_limited_delivered_bytes
.map_or(false, |app_limited_bytes| {
self.delivered_bytes > app_limited_bytes
})
{
// Clear app-limited field if bubble is ACKed and gone
self.app_limited_delivered_bytes = None;
}

//= https://tools.ietf.org/id/draft-cheng-iccrg-delivery-rate-estimation-02#3.3
//# UpdateRateSample() is invoked multiple times when a stretched ACK acknowledges
//# multiple data packets. In this case we use the information from the most recently
Expand Down
68 changes: 9 additions & 59 deletions quic/s2n-quic-core/src/recovery/bandwidth/estimator/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,23 +84,23 @@ fn on_packet_sent_timestamp_initialization() {
let mut bw_estimator = Estimator::default();

// Test that first_sent_time and delivered_time are updated on the first sent packet
let packet_info = bw_estimator.on_packet_sent(0, None, t0);
let packet_info = bw_estimator.on_packet_sent(0, false, t0);
assert_eq!(t0, packet_info.first_sent_time);
assert_eq!(t0, packet_info.delivered_time);
assert_eq!(Some(t0), bw_estimator.first_sent_time);
assert_eq!(Some(t0), bw_estimator.delivered_time);

// Test that first_sent_time and delivered_time are not updated if packets are in flight
let t1 = t0 + Duration::from_secs(1);
let packet_info = bw_estimator.on_packet_sent(1500, None, t1);
let packet_info = bw_estimator.on_packet_sent(1500, false, t1);
assert_eq!(t0, packet_info.first_sent_time);
assert_eq!(t0, packet_info.delivered_time);
assert_eq!(Some(t0), bw_estimator.first_sent_time);
assert_eq!(Some(t0), bw_estimator.delivered_time);

// Test that first_sent_time and delivered_time are updated after an idle period
let t2 = t0 + Duration::from_secs(2);
let packet_info = bw_estimator.on_packet_sent(0, None, t2);
let packet_info = bw_estimator.on_packet_sent(0, false, t2);
assert_eq!(t2, packet_info.first_sent_time);
assert_eq!(t2, packet_info.delivered_time);
assert_eq!(Some(t2), bw_estimator.first_sent_time);
Expand All @@ -116,66 +116,16 @@ fn on_packet_sent() {
delivered_time: Some(delivered_time),
lost_bytes: 100,
first_sent_time: Some(first_sent_time),
app_limited_delivered_bytes: None,
rate_sample: Default::default(),
};

let packet_info = bw_estimator.on_packet_sent(500, Some(true), first_sent_time);
let packet_info = bw_estimator.on_packet_sent(500, true, first_sent_time);
assert_eq!(first_sent_time, packet_info.first_sent_time);
assert_eq!(delivered_time, packet_info.delivered_time);
assert_eq!(15000, packet_info.delivered_bytes);
assert_eq!(100, packet_info.lost_bytes);
assert!(packet_info.is_app_limited);
assert_eq!(500, packet_info.bytes_in_flight);
assert_eq!(Some(500 + 15000), bw_estimator.app_limited_delivered_bytes);
}

#[test]
fn app_limited() {
let first_sent_time = NoopClock.get_time();
let delivered_time = first_sent_time + Duration::from_secs(1);
let mut bw_estimator = Estimator {
delivered_bytes: 15000,
delivered_time: Some(delivered_time),
lost_bytes: 100,
first_sent_time: Some(first_sent_time),
app_limited_delivered_bytes: None,
rate_sample: Default::default(),
};

// Packet is sent while app-limited, starting the app limited period
let packet_info = bw_estimator.on_packet_sent(1500, Some(true), first_sent_time);
assert!(packet_info.is_app_limited);
assert_eq!(Some(1500 + 15000), bw_estimator.app_limited_delivered_bytes);

// Packet is sent while not app-limited, but the app limited continues until all previous bytes in flight have been acknowledged
let packet_info = bw_estimator.on_packet_sent(500, Some(false), first_sent_time);
assert!(packet_info.is_app_limited);
assert_eq!(Some(1500 + 15000), bw_estimator.app_limited_delivered_bytes);

// Packet is sent while app-limited is not determined, but the app limited continues until all previous bytes in flight have been acknowledged
let packet_info = bw_estimator.on_packet_sent(500, None, first_sent_time);
assert!(packet_info.is_app_limited);
assert_eq!(Some(1500 + 15000), bw_estimator.app_limited_delivered_bytes);

let packet_info = PacketInfo {
delivered_bytes: bw_estimator.delivered_bytes,
delivered_time,
lost_bytes: 0,
first_sent_time,
bytes_in_flight: 1500,
is_app_limited: false,
};

// Acknowledge all the bytes that were inflight when the app-limited period began
bw_estimator.on_ack(1500, delivered_time, packet_info, delivered_time);
// Still app_limited, since we need bytes to be acknowledged after the app limited period
assert_eq!(Some(1500 + 15000), bw_estimator.app_limited_delivered_bytes);

// Acknowledge one more byte
bw_estimator.on_ack(1, delivered_time, packet_info, delivered_time);
// Now the app limited period is over
assert_eq!(None, bw_estimator.app_limited_delivered_bytes);
}

#[test]
Expand All @@ -187,11 +137,11 @@ fn on_packet_ack_rate_sample() {

// Send three packets. In between each send, other packets were acknowledged, and thus the
// delivered_bytes amount is increased.
let packet_1 = bw_estimator.on_packet_sent(0, Some(false), t0);
let packet_1 = bw_estimator.on_packet_sent(0, false, t0);
bw_estimator.delivered_bytes = 100000;
let packet_2 = bw_estimator.on_packet_sent(1500, Some(true), t1);
let packet_2 = bw_estimator.on_packet_sent(1500, true, t1);
bw_estimator.delivered_bytes = 200000;
let packet_3 = bw_estimator.on_packet_sent(3000, Some(false), t2);
let packet_3 = bw_estimator.on_packet_sent(3000, false, t2);

let now = t0 + Duration::from_secs(10);
let delivered_bytes = bw_estimator.delivered_bytes;
Expand Down Expand Up @@ -295,13 +245,13 @@ fn on_packet_ack_implausible_ack_rate() {
let mut bw_estimator = Estimator::default();

// A packet is sent and acknowledged 4 seconds later
let packet_info = bw_estimator.on_packet_sent(0, Some(false), t0);
let packet_info = bw_estimator.on_packet_sent(0, false, t0);
let t4 = t0 + Duration::from_secs(4);
bw_estimator.on_ack(1500, t0, packet_info, t4);

// A packet is sent and acknowledged 1 second later
let t5 = t0 + Duration::from_secs(5);
let packet_info = bw_estimator.on_packet_sent(1500, Some(false), t5);
let packet_info = bw_estimator.on_packet_sent(1500, false, t5);
let now = t0 + Duration::from_secs(6);
bw_estimator.on_ack(1500, t0 + Duration::from_secs(5), packet_info, now);

Expand Down
9 changes: 6 additions & 3 deletions quic/s2n-quic-core/src/recovery/bbr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,13 @@ impl CongestionController for BbrCongestionController {
&mut self,
time_sent: Timestamp,
sent_bytes: usize,
app_limited: Option<bool>,
_rtt_estimator: &RttEstimator,
) -> Self::PacketInfo {
let is_app_limited = false; // TODO: determine if app limited
let packet_info =
self.bw_estimator
.on_packet_sent(*self.bytes_in_flight, is_app_limited, time_sent);

if sent_bytes > 0 {
self.recovery_state.on_packet_sent();

Expand All @@ -111,8 +115,7 @@ impl CongestionController for BbrCongestionController {
.expect("sent_bytes should not exceed u32::MAX");
}

self.bw_estimator
.on_packet_sent(*self.bytes_in_flight, app_limited, time_sent)
packet_info
}

fn on_rtt_update(&mut self, _time_sent: Timestamp, _rtt_estimator: &RttEstimator) {
Expand Down
9 changes: 0 additions & 9 deletions quic/s2n-quic-core/src/recovery/congestion_controller.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,13 @@ pub trait CongestionController: 'static + Clone + Send + Debug {
/// the packet is acknowledged and the packet was the newest acknowledged in the ACK frame,
/// or to `on_packet_lost` if the packet was declared lost.
///
/// `app_limited` indicates whether the application has enough data to send to fill the
/// congestion window. This value will be `None` for Initial and Handshake packets.
///
/// Note: Sent bytes may be 0 in the case the packet being sent contains only ACK frames.
/// These pure ACK packets are not congestion-controlled to ensure congestion control
/// does not impede congestion feedback.
fn on_packet_sent(
&mut self,
time_sent: Timestamp,
sent_bytes: usize,
app_limited: Option<bool>,
rtt_estimator: &RttEstimator,
) -> Self::PacketInfo;

Expand Down Expand Up @@ -186,7 +182,6 @@ pub mod testing {
&mut self,
_time_sent: Timestamp,
_bytes_sent: usize,
_app_limited: Option<bool>,
_rtt_estimator: &RttEstimator,
) {
}
Expand Down Expand Up @@ -257,7 +252,6 @@ pub mod testing {
pub congestion_events: u32,
pub requires_fast_retransmission: bool,
pub loss_bursts: u32,
pub app_limited: Option<bool>,
}

impl Default for CongestionController {
Expand All @@ -274,7 +268,6 @@ pub mod testing {
congestion_events: 0,
requires_fast_retransmission: false,
loss_bursts: 0,
app_limited: None,
}
}
}
Expand Down Expand Up @@ -302,12 +295,10 @@ pub mod testing {
&mut self,
_time_sent: Timestamp,
bytes_sent: usize,
app_limited: Option<bool>,
_rtt_estimator: &RttEstimator,
) {
self.bytes_in_flight += bytes_sent as u32;
self.requires_fast_retransmission = false;
self.app_limited = app_limited;
}

fn on_rtt_update(&mut self, _time_sent: Timestamp, _rtt_estimator: &RttEstimator) {
Expand Down
10 changes: 1 addition & 9 deletions quic/s2n-quic-core/src/recovery/cubic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,6 @@ impl CongestionController for CubicCongestionController {
&mut self,
time_sent: Timestamp,
bytes_sent: usize,
app_limited: Option<bool>,
rtt_estimator: &RttEstimator,
) {
if bytes_sent == 0 {
Expand All @@ -205,14 +204,7 @@ impl CongestionController for CubicCongestionController {
.try_add(bytes_sent)
.expect("bytes sent should not exceed u32::MAX");

if let Some(app_limited) = app_limited {
// We check both the given `app_limited` value and is_congestion_window_under_utilized()
// as is_congestion_window_under_utilized() is more lenient with respect to the utilization
// of the congestion window than the app_limited check. is_congestion_window_under_utilized()
// returns true if there are more than 3 MTU's of space left in the cwnd, or less than
// half the cwnd is utilized in slow start.
self.under_utilized = app_limited && self.is_congestion_window_under_utilized();
}
self.under_utilized = self.is_congestion_window_under_utilized();

if let Recovery(recovery_start_time, RequiresTransmission) = self.state {
// A packet has been sent since we entered recovery (fast retransmission)
Expand Down
Loading

0 comments on commit 91bd325

Please sign in to comment.