From 6d2573b92e065074b447c926d150ddc5b88d7cf9 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Thu, 27 Aug 2026 17:59:17 +0200 Subject: [PATCH] perf: make the per-slice SkillStore compact instead of dense MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #17. Each `TimeSlice` owned a `Vec` indexed by the GLOBAL `Index.0`, so a slice's footprint was O(largest index it touches) rather than O(competitors in it). Two competitors at 19998/19999 reserved 20,000 slots per slice; the same games between indices 0 and 1 reserved two. The store is now a compact `Vec` plus a `HashMap` slot map and a parallel `Vec` for iteration. The hash is paid once at ingestion: each event's `Item` caches its slot, and the convergence loop reaches skills through `at`/`at_mut` by slot, so no hashing enters the hot path — which is the property the dense layout existed to provide. Measured on the issue's own workload (200 slices, one 1v1 each, 20,000-key roster, release, peak RSS): indices 0 / 1 52 MB -> 5.55 MB indices 19998 / 19999 309 MB -> 8.39 MB The 257 MB gap is now 2.8 MB, and that residual is CompetitorStore, which is also dense over the global index but is a single store for the whole history rather than one per slice — so it does not multiply. Left alone deliberately. Benchmarks, against the pre-change code: Batch::iteration +2.4% (regressed) history_converge x3 -18.8%, -21.6%, -21.7% (improved) The three convergence benchmarks are the realistic workload and they gain ~20% from the better locality of a compact store. The micro-benchmark loses 2.4% because `Item` grew eight bytes for the cached slot; `agent` cannot be dropped to compensate, since `within_prior` still needs the global index to reach the competitor's rating. I judged 2.4% on one micro-benchmark an acceptable price for ~20% on the real ones plus the memory fix, but it is a regression against #17's stated "no regression" criterion, so it is called out rather than buried. The regression test asserts on a new test-only `allocated_slots()`, not on `len()`. That distinction is load-bearing: the old dense store reported the true competitor count from `len()` while allocating max_index+1 slots, so a test written against `len()` would have passed on the defect. Mutation-proved by re-adding the dense padding, which fails it. One coupling is now pinned by a debug_assert: `filtered_step` clones events whose `Item`s carry slots resolved against the REAL store, so its scratch store must assign identical slots. It does, because `iter()` yields slot order and `insert` allocates in call order — but that is an invariant across two types, so it is asserted rather than left to be rediscovered. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T5SYDExxL4vZgvunrcNSMc --- src/history.rs | 43 +++++++++ src/storage/skill_store.rs | 174 +++++++++++++++++++++++++------------ src/time_slice.rs | 43 +++++++-- 3 files changed, 196 insertions(+), 64 deletions(-) diff --git a/src/history.rs b/src/history.rs index 912414f..fc7ae6b 100644 --- a/src/history.rs +++ b/src/history.rs @@ -1026,6 +1026,49 @@ mod tests { arena::ScratchArena, }; + /// #17: a slice's footprint must be O(competitors in the slice), not + /// O(largest global index it touches). The store used to be a dense + /// `Vec` indexed by `Index.0`, so the same two-competitor games cost + /// 20,000 slots per slice when the competitors sat at the top of a large + /// roster. Measured end to end, peak RSS was 309 MB against 52 MB. + #[test] + fn per_slice_footprint_is_independent_of_index_magnitude() { + fn total_skill_slots(high_indices: bool) -> usize { + let mut h: History = + History::builder_with_key().build(); + + for i in 0..2_000 { + h.intern(&format!("k{i:05}")); + } + + let (a, b) = if high_indices { + ("k01998".to_string(), "k01999".to_string()) + } else { + ("k00000".to_string(), "k00001".to_string()) + }; + + for time in 1..=20i64 { + h.record_winner(&a, &b, time).unwrap(); + } + + h.time_slices + .iter() + .map(|ts| ts.skills.allocated_slots()) + .sum() + } + + let low = total_skill_slots(false); + let high = total_skill_slots(true); + + assert_eq!(low, high, "footprint must not depend on index magnitude"); + + // A dense store over a 2,000-key roster would allocate 20 x 2,000. + assert!( + high < 1_000, + "20 slices of 2 competitors allocated {high} slots" + ); + } + fn make_events_1v1( pairs: &[(&'static str, &'static str)], outcomes: &[Outcome], diff --git a/src/storage/skill_store.rs b/src/storage/skill_store.rs index 00bcf21..14d846f 100644 --- a/src/storage/skill_store.rs +++ b/src/storage/skill_store.rs @@ -1,15 +1,27 @@ +use std::collections::HashMap; + use crate::{Index, time_slice::Skill}; -/// Dense Vec-backed store for per-agent skill state within a `TimeSlice`. +/// Compact per-slice store for skill state, addressed by a slice-local slot. /// -/// Indexed directly by Index.0, eliminating `HashMap` hashing in the inner -/// convergence loop. Uses a parallel `present` mask so iteration skips -/// absent slots without incurring per-slot Option overhead in the hot path. +/// `skills` holds one entry per competitor **in this slice**, so memory is +/// O(competitors in the slice). It used to be a dense `Vec` indexed by +/// the global `Index.0`, which made a slice's footprint O(largest index it +/// touches): a single 1v1 game between competitors 19998 and 19999 reserved +/// 20,000 slots. +/// +/// The dense layout existed to keep `HashMap` hashing out of the inner +/// convergence loop, and that property is preserved. `slots` is consulted only +/// while building a slice; every hot-path access goes through +/// [`SkillStore::at`] / [`SkillStore::at_mut`] with a slot resolved once at +/// ingestion and cached on the event's `Item`. #[derive(Debug, Default)] pub struct SkillStore { skills: Vec, - present: Vec, - n_present: usize, + /// Slot -> global index, parallel to `skills`, so iteration can report the + /// global index without a reverse lookup. + indices: Vec, + slots: HashMap, } impl SkillStore { @@ -17,73 +29,99 @@ impl SkillStore { Self::default() } - fn ensure_capacity(&mut self, idx: usize) { - if idx >= self.skills.len() { - self.skills.resize_with(idx + 1, Skill::default); - self.present.resize(idx + 1, false); - } + /// Resolve a global index to this slice's slot, if the competitor is here. + /// + /// This hashes. Call it at ingestion and cache the result; do not call it + /// from the convergence loop. + pub fn slot_of(&self, idx: Index) -> Option { + self.slots.get(&idx).copied() } - pub fn insert(&mut self, idx: Index, skill: Skill) { - self.ensure_capacity(idx.0); - if !self.present[idx.0] { - self.n_present += 1; + /// Skill at a slot resolved earlier by [`SkillStore::slot_of`]. + /// + /// # Panics + /// + /// Panics if `slot` is out of range, which means it came from a different + /// slice's store. + pub fn at(&self, slot: u32) -> &Skill { + &self.skills[slot as usize] + } + + /// Mutable counterpart to [`SkillStore::at`]. + /// + /// # Panics + /// + /// Panics if `slot` is out of range. + pub fn at_mut(&mut self, slot: u32) -> &mut Skill { + &mut self.skills[slot as usize] + } + + /// Insert or overwrite a competitor's skill, returning its slot. + pub fn insert(&mut self, idx: Index, skill: Skill) -> u32 { + match self.slots.get(&idx) { + Some(&slot) => { + self.skills[slot as usize] = skill; + slot + } + None => { + let slot = u32::try_from(self.skills.len()) + .expect("a time slice cannot hold more than u32::MAX competitors"); + + self.skills.push(skill); + self.indices.push(idx); + self.slots.insert(idx, slot); + + slot + } } - self.skills[idx.0] = skill; - self.present[idx.0] = true; } pub fn get(&self, idx: Index) -> Option<&Skill> { - if idx.0 < self.present.len() && self.present[idx.0] { - Some(&self.skills[idx.0]) - } else { - None - } - } - - /// Whether a slot is occupied. Test-only. - #[cfg(test)] - pub fn contains(&self, idx: Index) -> bool { - idx.0 < self.present.len() && self.present[idx.0] - } - - /// Number of occupied slots. Test-only. - #[cfg(test)] - pub fn len(&self) -> usize { - self.n_present + self.slot_of(idx).map(|slot| self.at(slot)) } pub fn get_mut(&mut self, idx: Index) -> Option<&mut Skill> { - if idx.0 < self.present.len() && self.present[idx.0] { - Some(&mut self.skills[idx.0]) - } else { - None - } + self.slot_of(idx) + .map(|slot| &mut self.skills[slot as usize]) } + /// Whether a competitor is present in this slice. Test-only. + #[cfg(test)] + pub fn contains(&self, idx: Index) -> bool { + self.slots.contains_key(&idx) + } + + /// Number of competitors in this slice. Test-only. + #[cfg(test)] + pub fn len(&self) -> usize { + self.skills.len() + } + + /// Slots actually allocated — the quantity #17 is about, and NOT the same + /// as `len` for every possible implementation. + /// + /// A store indexed by the global `Index` must report `max_index + 1` here + /// while reporting the true competitor count from `len`, which is exactly + /// how the original defect hid. Tests that mean to pin the footprint must + /// assert on this. + #[cfg(test)] + pub fn allocated_slots(&self) -> usize { + self.skills.len() + } + + /// Iterate in slot order — the order competitors were first seen in this + /// slice. Deterministic for a given event order, which is what the + /// cross-thread determinism test relies on. pub fn iter(&self) -> impl Iterator { - self.present.iter().enumerate().filter_map(|(i, &p)| { - if p { - Some((Index(i), &self.skills[i])) - } else { - None - } - }) + self.indices.iter().copied().zip(self.skills.iter()) } pub fn iter_mut(&mut self) -> impl Iterator { - self.skills - .iter_mut() - .zip(self.present.iter()) - .enumerate() - .filter_map(|(i, (s, &p))| if p { Some((Index(i), s)) } else { None }) + self.indices.iter().copied().zip(self.skills.iter_mut()) } pub fn keys(&self) -> impl Iterator + '_ { - self.present - .iter() - .enumerate() - .filter_map(|(i, &p)| if p { Some(Index(i)) } else { None }) + self.indices.iter().copied() } } @@ -109,7 +147,7 @@ mod tests { } #[test] - fn iter_skips_absent_slots() { + fn iter_reports_global_indices() { let mut store = SkillStore::new(); store.insert(Index(0), Skill::default()); store.insert(Index(5), Skill::default()); @@ -124,4 +162,28 @@ mod tests { store.insert(Index(2), Skill::default()); assert_eq!(store.len(), 1); } + + /// The defect in #17: a slice holding two competitors must cost the same + /// whether their indices are small or large. + #[test] + fn footprint_is_independent_of_index_magnitude() { + let mut low = SkillStore::new(); + low.insert(Index(0), Skill::default()); + low.insert(Index(1), Skill::default()); + + let mut high = SkillStore::new(); + high.insert(Index(19_998), Skill::default()); + high.insert(Index(19_999), Skill::default()); + + assert_eq!(low.len(), high.len()); + assert_eq!(low.skills.capacity(), high.skills.capacity()); + } + + #[test] + fn slot_survives_reinsert() { + let mut store = SkillStore::new(); + let first = store.insert(Index(7), Skill::default()); + let again = store.insert(Index(7), Skill::default()); + assert_eq!(first, again); + } } diff --git a/src/time_slice.rs b/src/time_slice.rs index 1f73d30..aa68985 100644 --- a/src/time_slice.rs +++ b/src/time_slice.rs @@ -51,6 +51,13 @@ pub enum EventKind { #[derive(Clone, Debug)] struct Item { agent: Index, + /// This competitor's slot in the owning slice's `SkillStore`, resolved + /// once at ingestion. + /// + /// The convergence loop reaches skills through this rather than through + /// `agent`, which is what keeps `HashMap` hashing out of the hot path now + /// that the store is compact rather than indexed by the global `Index`. + slot: u32, likelihood: Gaussian, } @@ -62,7 +69,7 @@ impl Item { agents: &CompetitorStore, ) -> Rating { let r = &agents[self.agent].rating; - let skill = skills.get(self.agent).unwrap(); + let skill = skills.at(self.slot); if forward { Rating::new(skill.forward, r.beta, r.drift) @@ -157,9 +164,9 @@ impl Event { for (t, team) in self.teams.iter_mut().enumerate() { for (i, item) in team.items.iter_mut().enumerate() { let fresh = update.likelihoods[t][i]; - let old_likelihood = skills.get(item.agent).unwrap().likelihood; + let old_likelihood = skills.at(item.slot).likelihood; let new_likelihood = (old_likelihood / item.likelihood) * fresh; - skills.get_mut(item.agent).unwrap().likelihood = new_likelihood; + skills.at_mut(item.slot).likelihood = new_likelihood; item.likelihood = fresh; } } @@ -297,14 +304,16 @@ impl TimeSlice { for idx in this_agent { let elapsed = compute_elapsed(agents[*idx].last_time.as_ref(), &self.time); + let forward = agents[*idx].receive(&self.time); + if let Some(skill) = self.skills.get_mut(*idx) { skill.elapsed = elapsed; - skill.forward = agents[*idx].receive(&self.time); + skill.forward = forward; } else { self.skills.insert( *idx, Skill { - forward: agents[*idx].receive(&self.time), + forward, backward: N_INF, likelihood: N_INF, elapsed, @@ -313,6 +322,8 @@ impl TimeSlice { } } + let skills = &self.skills; + let events = composition.iter().enumerate().map(|(e, event)| { let teams = event .iter() @@ -322,6 +333,11 @@ impl TimeSlice { .iter() .map(|&agent| Item { agent, + // Every participant was inserted into `skills` + // just above, so the slot always resolves. + slot: skills + .slot_of(agent) + .expect("participant must be present in the slice store"), likelihood: N_INF, }) .collect::>(); @@ -408,10 +424,10 @@ impl TimeSlice { for (t, team) in event.teams.iter_mut().enumerate() { for (i, item) in team.items.iter_mut().enumerate() { - let old_likelihood = self.skills.get(item.agent).unwrap().likelihood; + let old_likelihood = self.skills.at(item.slot).likelihood; let new_likelihood = (old_likelihood / item.likelihood) * g.likelihoods[t][i]; - self.skills.get_mut(item.agent).unwrap().likelihood = new_likelihood; + self.skills.at_mut(item.slot).likelihood = new_likelihood; item.likelihood = g.likelihoods[t][i]; } } @@ -633,7 +649,7 @@ impl TimeSlice { None => rating.prior, }; - scratch.skills.insert( + let slot = scratch.skills.insert( agent, Skill { forward, @@ -642,6 +658,17 @@ impl TimeSlice { elapsed: skill.elapsed, }, ); + + // The cloned events carry slots resolved against the REAL store, so + // the scratch must assign the same ones. It does because `iter()` + // yields slot order and `insert` allocates slots in call order — + // but that is a coupling between two types, so pin it here rather + // than leave it to be rediscovered after it breaks. + debug_assert_eq!( + Some(slot), + self.skills.slot_of(agent), + "scratch slot must match the real slice's slot for {agent:?}" + ); } scratch.iterate_to_convergence(agents);