refactor(s2n-quic-core): represent Bandwidth as nanos per byte (#1439)

* refactor(s2n-quic-core): represent Bandwidth as nanos per byte

* Add comment

* Add comment on max value

Author: WesleyRosenblum

quic/s2n-quic-core/src/recovery/bandwidth/estimator.rs

@@ -2,8 +2,12 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use crate::time::Timestamp;
-use core::{cmp::max, time::Duration};
+use core::{
+    cmp::{max, Ordering},
+    time::Duration,
+};
 use num_rational::Ratio;
+use num_traits::Inv;
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
 /// Bandwidth-related data tracked for each sent packet
@@ -26,45 +30,73 @@ pub struct PacketInfo {
     pub is_app_limited: bool,
 }
 
-const MICRO_BITS_PER_BYTE: u64 = 8 * 1000000;
-
-#[derive(Copy, Clone, Debug, Default, Eq, Ord, PartialOrd, PartialEq)]
+/// Represents a rate at which data is transferred
+///
+/// While bandwidth is typically thought of as an amount of data over a fixed
+/// amount of time (bytes per second, for example), in this case we internally
+/// represent bandwidth as the inverse: an amount of time to send a fixed amount
+/// of data (nanoseconds per byte, in this case). This allows for some of the  
+/// math operations needed on `Bandwidth` to avoid division, while reducing the
+/// likelihood of panicking due to overflow.
+///
+/// The maximum (non-infinite) value that can be represented is 1 GB/second.
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
 pub struct Bandwidth {
-    bits_per_second: u64,
+    nanos_per_byte: u64,
 }
 
 impl Bandwidth {
-    pub const ZERO: Bandwidth = Bandwidth { bits_per_second: 0 };
-
-    pub const MAX: Bandwidth = Bandwidth {
-        bits_per_second: u64::MAX,
+    pub const ZERO: Bandwidth = Bandwidth {
+        nanos_per_byte: u64::MAX,
     };
 
+    pub const INFINITY: Bandwidth = Bandwidth { nanos_per_byte: 0 };
+
     /// Constructs a new `Bandwidth` with the given bytes per interval
     pub const fn new(bytes: u64, interval: Duration) -> Self {
-        if interval.is_zero() {
+        if interval.is_zero() || bytes == 0 {
             Bandwidth::ZERO
         } else {
             Self {
-                // Prefer multiplying by MICRO_BITS_PER_BYTE first to avoid losing resolution
-                bits_per_second: match bytes.checked_mul(MICRO_BITS_PER_BYTE) {
-                    Some(micro_bits) => micro_bits / interval.as_micros() as u64,
-                    None => {
-                        // If that overflows, divide first by the interval
-                        (bytes / interval.as_micros() as u64).saturating_mul(MICRO_BITS_PER_BYTE)
-                    }
-                },
+                nanos_per_byte: interval.as_nanos() as u64 / bytes,
             }
         }
     }
 }
 
+impl Default for Bandwidth {
+    fn default() -> Self {
+        Bandwidth::ZERO
+    }
+}
+
+impl core::cmp::PartialOrd for Bandwidth {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl core::cmp::Ord for Bandwidth {
+    fn cmp(&self, other: &Self) -> Ordering {
+        // The higher the nano_per_byte, the lower the bandwidth,
+        // so reverse the ordering when comparing
+        self.nanos_per_byte.cmp(&other.nanos_per_byte).reverse()
+    }
+}
+
 impl core::ops::Mul<Ratio<u64>> for Bandwidth {
     type Output = Bandwidth;
 
     fn mul(self, rhs: Ratio<u64>) -> Self::Output {
+        if self == Bandwidth::ZERO {
+            return Bandwidth::ZERO;
+        }
+
         Bandwidth {
-            bits_per_second: (rhs * self.bits_per_second).to_integer(),
+            // Since `Bandwidth` is represented as time/byte and not bytes/time, we should divide
+            // by the given ratio to result in a higher nanos_per_byte value (lower bandwidth).
+            // To avoid division, we can multiply by the inverse of the ratio instead
+            nanos_per_byte: (rhs.inv() * self.nanos_per_byte).to_integer(),
         }
     }
 }
@@ -73,14 +105,10 @@ impl core::ops::Mul<Duration> for Bandwidth {
     type Output = u64;
 
     fn mul(self, rhs: Duration) -> Self::Output {
-        // Prefer multiplying by the duration first to avoid losing resolution
-        match self.bits_per_second.checked_mul(rhs.as_micros() as u64) {
-            Some(micro_bits) => micro_bits / MICRO_BITS_PER_BYTE,
-            None => {
-                // If that overflows, divide first by MICRO_BITS_PER_BYTE
-                (self.bits_per_second / MICRO_BITS_PER_BYTE).saturating_mul(rhs.as_micros() as u64)
-            }
+        if self == Bandwidth::INFINITY {
+            return u64::MAX;
         }
+        rhs.as_nanos() as u64 / self.nanos_per_byte
     }
 }
 
@@ -94,7 +122,7 @@ impl core::ops::Div<Bandwidth> for u64 {
     type Output = Duration;
 
     fn div(self, rhs: Bandwidth) -> Self::Output {
-        Duration::from_micros(self * MICRO_BITS_PER_BYTE / rhs.bits_per_second)
+        Duration::from_nanos(rhs.nanos_per_byte.saturating_mul(self))
     }
 }
 

quic/s2n-quic-core/src/recovery/bandwidth/estimator/tests.rs

@@ -8,24 +8,15 @@ use crate::time::{Clock, NoopClock};
 fn bandwidth() {
     let result = Bandwidth::new(1000, Duration::from_secs(10));
 
-    assert_eq!(100 * 8, result.bits_per_second);
+    // 10 seconds = 10^10 nanoseconds, nanos_per_byte = 10^10 / 1000 = 10_000_000
+    assert_eq!(10_000_000, result.nanos_per_byte);
 }
 
 #[test]
 fn bandwidth_saturating() {
     let result = Bandwidth::new(u64::MAX, Duration::from_secs(1));
 
-    assert_eq!(Bandwidth::MAX, result);
-}
-
-#[test]
-fn bandwidth_overflow() {
-    let result = Bandwidth::new(u64::MAX, Duration::from_secs(8));
-
-    let expected_bits_per_second =
-        u64::MAX / (Duration::from_secs(8).as_micros() as u64) * MICRO_BITS_PER_BYTE;
-
-    assert_eq!(expected_bits_per_second, result.bits_per_second);
+    assert_eq!(Bandwidth::INFINITY, result);
 }
 
 #[test]
@@ -44,6 +35,11 @@ fn bandwidth_mul_ratio() {
     assert_eq!(result, Bandwidth::new(3000, Duration::from_secs(1)));
 }
 
+#[test]
+fn bandwidth_zero_mul_ratio() {
+    assert_eq!(Bandwidth::ZERO, Bandwidth::ZERO * Ratio::new(3, 7));
+}
+
 #[test]
 fn bandwidth_mul_duration() {
     let bandwidth = Bandwidth::new(7000, Duration::from_secs(2));
@@ -55,25 +51,13 @@ fn bandwidth_mul_duration() {
 
 #[test]
 fn bandwidth_mul_saturation() {
-    let bandwidth = Bandwidth::MAX;
+    let bandwidth = Bandwidth::INFINITY;
 
     let result = bandwidth * Duration::from_secs(10);
 
     assert_eq!(result, u64::MAX);
 }
 
-#[test]
-fn bandwidth_mul_overflow() {
-    let bandwidth = Bandwidth {
-        bits_per_second: 18446744073704000000, // closest value to u64::MAX that divides evenly by MICRO_BITS_PER_BYTE
-    };
-
-    let result = bandwidth * Duration::from_millis(500);
-
-    // Divide by 8 to convert to bytes and divide by 2 since we multiplied by .5 seconds
-    assert_eq!(result, 18446744073704000000 / 8 / 2);
-}
-
 #[test]
 fn u64_div_bandwidth() {
     let bandwidth = Bandwidth::new(10_000, Duration::from_secs(1));
@@ -85,6 +69,15 @@ fn u64_div_bandwidth() {
     assert_eq!(bytes / bandwidth, Duration::from_millis(200));
 }
 
+#[test]
+fn bandwidth_ordering() {
+    let low = Bandwidth::new(10_000, Duration::from_secs(1));
+    let high = Bandwidth::new(20_000, Duration::from_secs(1));
+
+    assert!(high > low);
+    assert_eq!(high, low.max(high));
+}
+
 // first_sent_time and delivered_time typically hold values from recently acknowledged packets. However,
 // when  no packet has been sent yet, or there are no packets currently in flight, these values are initialized
 // with the time when a packet is sent. This test confirms first_sent_time and delivered_time are

quic/s2n-quic-core/src/recovery/bbr/congestion.rs

@@ -315,7 +315,7 @@ mod tests {
             loss_round_counter: Default::default(),
             loss_in_round: true,
             inflight_latest: 100,
-            bw_latest: Bandwidth::MAX,
+            bw_latest: Bandwidth::INFINITY,
         };
 
         state.reset();

quic/s2n-quic-core/src/recovery/bbr/data_rate.rs

@@ -48,8 +48,8 @@ impl Model {
 
         Self {
             max_bw_filter: WindowedMaxFilter::new(MAX_BW_FILTER_LEN),
-            bw_hi: Bandwidth::MAX,
-            bw_lo: Bandwidth::MAX,
+            bw_hi: Bandwidth::INFINITY,
+            bw_lo: Bandwidth::INFINITY,
             bw: Bandwidth::ZERO,
             cycle_count: Default::default(),
         }
@@ -121,7 +121,7 @@ impl Model {
         //# BBRInitLowerBounds():
         //#   if (BBR.bw_lo == Infinity)
         //#     BBR.bw_lo = BBR.max_bw
-        if self.bw_lo == Bandwidth::MAX {
+        if self.bw_lo == Bandwidth::INFINITY {
             self.bw_lo = self.max_bw()
         }
 
@@ -137,7 +137,7 @@ impl Model {
         //= https://tools.ietf.org/id/draft-cardwell-iccrg-bbr-congestion-control-02#4.5.6.3
         //# BBRResetLowerBounds():
         //#   BBR.bw_lo       = Infinity
-        self.bw_lo = Bandwidth::MAX
+        self.bw_lo = Bandwidth::INFINITY
     }
 
     /// Bounds `bw` to min(`max_bw`, `bw_lo`, `bw_hi)
@@ -160,8 +160,8 @@ mod tests {
 
         assert_eq!(Bandwidth::ZERO, model.max_bw());
         assert_eq!(Bandwidth::ZERO, model.bw());
-        assert_eq!(Bandwidth::MAX, model.bw_hi());
-        assert_eq!(Bandwidth::MAX, model.bw_lo());
+        assert_eq!(Bandwidth::INFINITY, model.bw_hi());
+        assert_eq!(Bandwidth::INFINITY, model.bw_lo());
     }
 
     #[test]
@@ -246,7 +246,7 @@ mod tests {
 
         // Resetting the lower bound sets bw_lo to Bandwidth::MAX
         model.reset_lower_bound();
-        assert_eq!(Bandwidth::MAX, model.bw_lo());
+        assert_eq!(Bandwidth::INFINITY, model.bw_lo());
     }
 
     #[test]

quic/s2n-quic-core/src/recovery/bbr/probe_bw.rs

@@ -905,7 +905,7 @@ mod tests {
         assert_eq!(CyclePhase::Refill, state.cycle_phase());
         // Lower bounds are reset
         assert_eq!(u64::MAX, data_volume_model.inflight_lo());
-        assert_eq!(Bandwidth::MAX, data_rate_model.bw_lo());
+        assert_eq!(Bandwidth::INFINITY, data_rate_model.bw_lo());
 
         assert_eq!(0, state.bw_probe_up_rounds);
         assert_eq!(0, state.bw_probe_up_acks);