From fcc832424ad15cc48a4256426e3fd2c7f77a81c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 17 Sep 2026 09:44:05 +0200 Subject: [PATCH] perf: allocate a ring's slots zeroed instead of writing every one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Queue::new built each slot with map/collect, which writes state into all of them and makes the whole ring resident at construction. WorkPort sizes its ring from max_operations, and a host that legitimately sets that to 32 768 — one armed read per connection, for 32k connections — paid 3.25 MiB of resident memory in a process that may never submit a single blocking job. The capacity CANNOT simply be reduced: every blocking job holds a core operation credit until its result is delivered, so undelivered results really can reach max_operations, and push is an assert!. Sizing the ring by the pool's thread count would put a panic in a shipped binary; #88's original suggestion, mine, was wrong about this and is corrected there. This does not touch the capacity. It observes that Slot's empty state IS the all-zero bit pattern — state starts at 0, value is MaybeUninit — so the slots can come from alloc_zeroed with no writes at all, and a zeroed allocation that large is fresh pages the OS faults in lazily. Every bound that depends on capacity, the blocking pool's cannot-overflow invariant included, is unchanged; only the pages the ring has actually used are resident. Nothing is allocated at run time, so the zero-allocation operation contracts are untouched. Measured with a one-Loop-per-process probe (RSS is a high-water mark, so building several in one process makes every later reading inherit the earlier ones), RSS delta across Loop::new: a 64 x 16 KiB / 32 768-op host profile 4800 KiB -> 1472 KiB (-3.25 MiB) the same with pooled_buffers = 0 3792 KiB -> 448 KiB loom keeps the per-slot construction, since its AtomicUsize and UnsafeCell are instrumented types whose representation is not all-zero. Two tests cover the consequences rather than the argument: every slot usable, the bound still exactly at capacity, no unwritten slot readable as a value, and a partly filled ring dropping exactly its own values and not 1024 zero slots. Sabotage-checked by filling the allocation with 0xAB, which fails them and four existing tests. Refs #88. --- crates/turnloop/src/queue.rs | 124 +++++++++++++++++++++++++++++++++-- crates/turnloop/src/sync.rs | 5 ++ 2 files changed, 123 insertions(+), 6 deletions(-) diff --git a/crates/turnloop/src/queue.rs b/crates/turnloop/src/queue.rs index 3f65da4..f2fabe5 100644 --- a/crates/turnloop/src/queue.rs +++ b/crates/turnloop/src/queue.rs @@ -18,16 +18,54 @@ pub(crate) struct Queue { unsafe impl Sync for Queue {} // SAFETY: no slot is externally borrowed; only Send values can cross threads. unsafe impl Send for Queue {} +/// A ring's slots, without touching them. +/// +/// `Slot`'s initial value **is** the all-zero bit pattern: `state` starts at 0, +/// which is the empty state, and `value` is a `MaybeUninit` for which every +/// pattern is valid. Building the slots with `map`/`collect` writes each one, +/// which makes the entire ring resident at construction — 3.15 MiB for a +/// 32 768-slot `WorkPort`, in a process that may never submit a blocking job. +/// +/// Asking the allocator for zeroed memory instead gives a ready ring with no +/// writes at all, and a zeroed allocation this large is fresh pages the OS +/// faults in lazily. The capacity is unchanged, so every bound that depends on +/// it — including the blocking pool's "cannot overflow" credit invariant — is +/// untouched; only the pages the ring has actually used are resident. +/// +/// `Queue::drop` pops until empty, and an untouched slot is state 0 (empty), so +/// no unwritten slot is ever read as a value. +#[cfg(not(loom))] +fn zeroed_slots(capacity: usize) -> Box<[Slot]> { + let layout = std::alloc::Layout::array::>(capacity).expect("ring layout"); + // SAFETY: `capacity >= 2` so the layout is non-zero-sized, and the all-zero + // bit pattern is a valid `Slot` as argued above. + unsafe { + let ptr = std::alloc::alloc_zeroed(layout).cast::>(); + if ptr.is_null() { + std::alloc::handle_alloc_error(layout); + } + Box::from_raw(std::ptr::slice_from_raw_parts_mut(ptr, capacity)) + } +} + +/// Under loom, `AtomicUsize` and `UnsafeCell` are instrumented types whose +/// representation is loom's business and is not all-zero, so the model build +/// keeps building each slot. +#[cfg(loom)] +fn zeroed_slots(capacity: usize) -> Box<[Slot]> { + (0..capacity) + .map(|_| Slot { + state: AtomicUsize::new(0), + value: UnsafeCell::new(MaybeUninit::uninit()), + }) + .collect() +} + impl Queue { pub fn new(capacity: usize) -> Self { assert!(capacity >= 2 && capacity.is_power_of_two()); Self { - slots: (0..capacity) - .map(|_| Slot { - state: AtomicUsize::new(0), - value: UnsafeCell::new(MaybeUninit::uninit()), - }) - .collect(), + slots: zeroed_slots(capacity), enqueue: AtomicUsize::new(0), dequeue: AtomicUsize::new(0), occupied: AtomicUsize::new(0), @@ -138,3 +176,77 @@ mod publication_models { }); } } + +#[cfg(all(test, not(loom)))] +mod zeroed_ring { + use super::*; + + /// A ring whose slots were never written must behave exactly like one whose + /// slots were constructed individually. + /// + /// The slots come from `alloc_zeroed` so that a large ring costs only the + /// pages it touches — `WorkPort` sizes its ring from `max_operations`, which + /// a host legitimately sets to 32 768, and writing every slot made all + /// 3.15 MiB of it resident in a process that may never submit a blocking + /// job. That is only sound because `Slot`'s empty state IS the all-zero bit + /// pattern, so this checks the consequences rather than the argument: + /// every slot is usable, the ring still refuses at exactly its capacity, + /// and nothing unwritten is ever read back as a value. + #[test] + fn an_unwritten_slot_is_empty_usable_and_bounded() { + const CAP: usize = 64; + let q: Queue = Queue::new(CAP); + assert!(q.is_empty(), "a freshly zeroed ring reads as empty"); + assert_eq!(q.pop(), None, "no unwritten slot may read back as a value"); + + // Every slot is usable, including ones no constructor ever touched. + for i in 0..CAP { + q.push(i) + .unwrap_or_else(|_| panic!("slot {i} must accept a value")); + } + // And the capacity still binds at exactly the declared size — the bound + // the blocking pool's "cannot overflow" credit invariant relies on. + assert_eq!(q.push(CAP), Err(CAP), "a full ring returns the value"); + + let mut seen = vec![false; CAP]; + for _ in 0..CAP { + let v = q.pop().expect("every pushed value comes back"); + assert!(!seen[v], "value {v} delivered twice"); + seen[v] = true; + } + assert!(seen.into_iter().all(|s| s), "every value was delivered"); + assert_eq!(q.pop(), None); + assert!(q.is_empty()); + + // Recycled slots still work after the ring has wrapped. + for i in 0..CAP { + q.push(i).expect("recycled slot"); + } + assert_eq!(q.push(CAP), Err(CAP)); + } + + /// Dropping a ring with values still in it must drop those values and no + /// others — an untouched zero slot must not be read as an initialised `T`. + #[test] + fn dropping_a_partly_filled_ring_drops_only_its_values() { + use std::sync::atomic::{AtomicUsize, Ordering}; + static DROPPED: AtomicUsize = AtomicUsize::new(0); + struct Counted; + impl Drop for Counted { + fn drop(&mut self) { + DROPPED.fetch_add(1, Ordering::SeqCst); + } + } + { + let q: Queue = Queue::new(1024); + for _ in 0..3 { + q.push(Counted).map_err(|_| ()).expect("slot"); + } + } + assert_eq!( + DROPPED.load(Ordering::SeqCst), + 3, + "exactly the three pushed values are dropped, not 1024 zero slots" + ); + } +} diff --git a/crates/turnloop/src/sync.rs b/crates/turnloop/src/sync.rs index cbd3548..e63cba2 100644 --- a/crates/turnloop/src/sync.rs +++ b/crates/turnloop/src/sync.rs @@ -14,6 +14,11 @@ pub(crate) use std::sync::{ pub(crate) struct UnsafeCell(std::cell::UnsafeCell); #[cfg(not(loom))] impl UnsafeCell { + // The queue's slots are allocated zeroed rather than constructed one at a + // time, so nothing in the non-loom build calls this any more. Kept because + // it is half of this shim's parity with loom's `UnsafeCell`, and a shim + // that only sometimes mirrors its subject is worse than an unused fn. + #[allow(dead_code)] pub fn new(v: T) -> Self { Self(std::cell::UnsafeCell::new(v)) }