From 5dd0c47159868d3ee190f8d0f701b81c6cc39a03 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Thu, 12 Dec 2024 20:08:01 -0700 Subject: [PATCH] fix(s2n-quic-dc): make debug assertions cheaper for TCP accept manager (#2419) --- .../src/stream/server/tokio/tcp/manager.rs | 30 +++---- .../stream/server/tokio/tcp/manager/list.rs | 87 ++++++++++++++----- 2 files changed, 80 insertions(+), 37 deletions(-) diff --git a/dc/s2n-quic-dc/src/stream/server/tokio/tcp/manager.rs b/dc/s2n-quic-dc/src/stream/server/tokio/tcp/manager.rs index adbaa2408..18f3ad604 100644 --- a/dc/s2n-quic-dc/src/stream/server/tokio/tcp/manager.rs +++ b/dc/s2n-quic-dc/src/stream/server/tokio/tcp/manager.rs @@ -106,7 +106,7 @@ where let capacity = workers.len(); let mut free = List::default(); for idx in 0..capacity { - free.push(&mut workers, idx); + free.push_back(&mut workers, idx); } let by_sojourn_time = List::default(); @@ -186,7 +186,7 @@ where self.inner .by_sojourn_time - .push(&mut self.inner.workers, idx); + .push_back(&mut self.inner.workers, idx); // kick off the initial poll to register wakers with the socket self.inner.poll_worker(idx, cx, publisher, clock); @@ -282,7 +282,8 @@ where // the worker is all done so indicate we have another free slot self.by_sojourn_time.remove(&mut self.workers, idx); - self.free.push(&mut self.workers, idx); + // use `push_front` instead to avoid cache churn + self.free.push_front(&mut self.workers, idx); cf } @@ -293,7 +294,7 @@ where C: Clock, { // if we have a free worker then use that - if let Some(idx) = self.free.pop(&mut self.workers) { + if let Some(idx) = self.free.pop_front(&mut self.workers) { trace!(op = %"next_worker", free = idx); return Some(idx); } @@ -304,7 +305,7 @@ where // if the worker's sojourn time exceeds the maximum, then reclaim it if sojourn >= self.max_sojourn_time() { trace!(op = %"next_worker", injected = idx, ?sojourn); - return self.by_sojourn_time.pop(&mut self.workers); + return self.by_sojourn_time.pop_front(&mut self.workers); } trace!(op = %"next_worker", ?sojourn, max_sojourn_time = ?self.max_sojourn_time()); @@ -317,18 +318,13 @@ where #[cfg(debug_assertions)] fn invariants(&self) { - for idx in 0..self.workers.len() { - assert!( - self.free - .iter(&self.workers) - .chain(self.by_sojourn_time.iter(&self.workers)) - .filter(|v| *v == idx) - .count() - == 1, - "worker {idx} should be linked at all times\n{:?}", - self.workers[idx].link, - ); - } + let mut linked_workers = self + .free + .iter(&self.workers) + .chain(self.by_sojourn_time.iter(&self.workers)) + .collect::>(); + linked_workers.sort(); + assert!((0..self.workers.len()).eq(linked_workers.iter().copied())); let mut expected_free_len = 0usize; for idx in self.free.iter(&self.workers) { diff --git a/dc/s2n-quic-dc/src/stream/server/tokio/tcp/manager/list.rs b/dc/s2n-quic-dc/src/stream/server/tokio/tcp/manager/list.rs index 6b1ba746e..d234bff66 100644 --- a/dc/s2n-quic-dc/src/stream/server/tokio/tcp/manager/list.rs +++ b/dc/s2n-quic-dc/src/stream/server/tokio/tcp/manager/list.rs @@ -46,7 +46,7 @@ impl List { } #[inline] - pub fn pop(&mut self, entries: &mut [L]) -> Option + pub fn pop_front(&mut self, entries: &mut [L]) -> Option where L: AsMut, { @@ -80,7 +80,30 @@ impl List { } #[inline] - pub fn push(&mut self, entries: &mut [L], idx: usize) + pub fn push_front(&mut self, entries: &mut [L], idx: usize) + where + L: AsMut, + { + debug_assert!(idx < usize::MAX); + + let head = self.head; + if head != usize::MAX { + entries[head].as_mut().prev = idx; + } else { + debug_assert!(self.is_empty()); + self.tail = idx; + } + self.head = idx; + + let link = entries[idx].as_mut(); + link.prev = usize::MAX; + link.next = head; + + self.set_linked_status(idx, true); + } + + #[inline] + pub fn push_back(&mut self, entries: &mut [L], idx: usize) where L: AsMut, { @@ -237,15 +260,31 @@ mod tests { impl CheckedList { #[inline] fn pop(&mut self, entries: &mut [Link]) -> Option { - let v = self.list.pop(entries); + let v = self.list.pop_front(entries); assert_eq!(v, self.oracle.pop_front()); self.invariants(entries); v } #[inline] - fn push(&mut self, entries: &mut [Link], v: usize) { - self.list.push(entries, v); + fn push(&mut self, entries: &mut [Link], v: usize, front: bool) { + if front { + self.push_front(entries, v); + } else { + self.push_back(entries, v); + } + } + + #[inline] + fn push_front(&mut self, entries: &mut [Link], v: usize) { + self.list.push_front(entries, v); + self.oracle.push_front(v); + self.invariants(entries); + } + + #[inline] + fn push_back(&mut self, entries: &mut [Link], v: usize) { + self.list.push_back(entries, v); self.oracle.push_back(v); self.invariants(entries); } @@ -279,7 +318,7 @@ mod tests { let locations = (0..LEN).map(|_| Location::A).collect(); for idx in 0..LEN { - a.push(&mut entries, idx); + a.push_back(&mut entries, idx); } Self { @@ -293,34 +332,34 @@ mod tests { impl Harness { #[inline] - fn transfer(&mut self, idx: usize) { + fn transfer(&mut self, idx: usize, front: bool) { let location = &mut self.locations[idx]; match location { Location::A => { self.a.remove(&mut self.entries, idx); - self.b.push(&mut self.entries, idx); + self.b.push(&mut self.entries, idx, front); *location = Location::B; } Location::B => { self.b.remove(&mut self.entries, idx); - self.a.push(&mut self.entries, idx); + self.a.push(&mut self.entries, idx, front); *location = Location::A; } } } #[inline] - fn pop_a(&mut self) { + fn pop_a(&mut self, front: bool) { if let Some(v) = self.a.pop(&mut self.entries) { - self.b.push(&mut self.entries, v); + self.b.push(&mut self.entries, v, front); self.locations[v] = Location::B; } } #[inline] - fn pop_b(&mut self) { + fn pop_b(&mut self, front: bool) { if let Some(v) = self.b.pop(&mut self.entries) { - self.a.push(&mut self.entries, v); + self.a.push(&mut self.entries, v, front); self.locations[v] = Location::A; } } @@ -328,9 +367,17 @@ mod tests { #[derive(Clone, Copy, Debug, TypeGenerator)] enum Op { - Transfer(#[generator(0..LEN)] usize), - PopA, - PopB, + Transfer { + #[generator(0..LEN)] + idx: usize, + front: bool, + }, + PopA { + front: bool, + }, + PopB { + front: bool, + }, } #[test] @@ -338,10 +385,10 @@ mod tests { check!().with_type::>().for_each(|ops| { let mut harness = Harness::default(); for op in ops { - match op { - Op::Transfer(idx) => harness.transfer(*idx), - Op::PopA => harness.pop_a(), - Op::PopB => harness.pop_b(), + match *op { + Op::Transfer { idx, front } => harness.transfer(idx, front), + Op::PopA { front } => harness.pop_a(front), + Op::PopB { front } => harness.pop_b(front), } } })