From b75d45b1d41a57088019721a3f46a40de8b21cb4 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 15 Jul 2026 11:19:03 +0100 Subject: [PATCH] perf(themes): PR #120 round 5 -- O(1) frozen counts via store totals The round-4 freeze read counted the retained for_uri vector inside status_facts_msg -- correct, but StatusFacts runs at frame cadence for every semantic session under the shared store mutex, so a long stale interval cost O(frames x diagnostics x sessions). DiagnosticStore now maintains per-URI severity totals alongside by_uri: set replaces them (an empty publication removes them with the vector), clear removes them, and mark_stale deliberately preserves both -- entries exist exactly when by_uri entries do, and set/clear are the store's only by_uri mutators. status_facts_msg reads the tuple in O(1), and the all-URI severity_totals sum reuses the cached tuples. Store unit (acceptance item 34) pins the invariant: all four totals replace correctly, survive staleness, and clear with the diagnostic vector; empty_set_clears_uri asserts the totals drop too. The rounds 3-4 freeze acceptance passes unchanged -- behavior parity, so the unit pin is the evidence (no runtime bite exists for a behavior-preserving refactor). Round-5 implementation user-authored; this commit folds it with framing revision 9, the protocol doc's O(1) note, and the acceptance manifest pointer to the item-34 unit. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VoiEyuPjoBhvwACf8HAnLB --- docs/semantic-frontend-protocol.md | 4 +- docs/theme-faces-framing.md | 32 +++++++++--- src/diag.rs | 82 ++++++++++++++++++++++++++---- src/semantic_render.rs | 43 ++++++++-------- tests/theme_faces_acceptance.rs | 3 +- 5 files changed, 124 insertions(+), 40 deletions(-) diff --git a/docs/semantic-frontend-protocol.md b/docs/semantic-frontend-protocol.md index 7c9d440..3258fb7 100644 --- a/docs/semantic-frontend-protocol.md +++ b/docs/semantic-frontend-protocol.md @@ -132,7 +132,9 @@ instance's stale-store diagnostic-count freeze is store knowledge, not session state — the re-sent `StatusFacts` after a snapshot carries the frozen counts, never zeros, including for a session whose first frame lands during staleness (a late joiner attaching -mid-edit). +mid-edit). The store maintains those per-URI severity totals with +the retained vector, so frame-time producers read them in O(1) +rather than rescanning diagnostics for every attached session. ## Capability and version mechanics diff --git a/docs/theme-faces-framing.md b/docs/theme-faces-framing.md index 1a5bc36..0a00b3d 100644 --- a/docs/theme-faces-framing.md +++ b/docs/theme-faces-framing.md @@ -1,7 +1,18 @@ # Theme faces — framing (Arc 4 stage 1, themes) -**Revision 8 — 2026-07-15. Status: implemented on branch -`theme-faces` (PR #120); revision 8 folds PR round 4.** +**Revision 9 — 2026-07-15. Status: implemented on branch +`theme-faces` (PR #120); revision 9 folds PR round 5.** + +Revision 9 (PR #120 round 5, one finding): the store-sourced freeze +no longer rescans the retained diagnostic vector at frame cadence. +`DiagnosticStore` maintains per-URI severity totals alongside +`by_uri`, replacing or removing both in `set` / `clear` while +`mark_stale` deliberately preserves both. `status_facts_msg` reads +that tuple in O(1), so the round-4 late-join correctness does not +turn a long stale interval into O(frames x diagnostics x semantic +sessions) work under the shared store mutex. Acceptance item 34 is +the store unit pin: all four totals replace correctly, survive +staleness, and clear with the diagnostic vector. Revision 8 (PR #120 round 4, one finding): the diagnostic-count freeze is sourced from the diag store itself, superseding round 3's @@ -671,10 +682,13 @@ pub struct ThemeFace { other buffers' baselines (each buffer's own snapshot precedes its revisit). The diagnostic-count freeze needs no protection here (rounds 3–4): it is sourced from the diag store's retained vector - — `mark_stale` keeps the last published diagnostics, whose counts - merely lag — never from session state, so the reset cannot zero - mid-edit counts and the freeze holds even for a session attaching - during staleness. Resetting on a failed write is harmless — the + and its cached per-URI severity tuple — `mark_stale` keeps the last + published diagnostics and counts, which merely lag — never from + session state, so the reset cannot zero mid-edit counts and the + freeze holds even for a session attaching during staleness. The + producer reads the tuple in O(1), rather than scanning the retained + vector for every semantic session at frame cadence (round 5). + Resetting on a failed write is harmless — the failure mode is one redundant re-send, never staleness. The GPU's `BufferSnapshot` arm mirrors the contract (round 3 finding 1): it clears its buffer-scoped facts — search/menu popups (which gate key and pointer interception) and @@ -1016,3 +1030,9 @@ Keybinding-driven tests dispatch keys, never `pmacs.command.invoke`. `SemanticRenderState` — the session's first frame reports the store's preserved counts, because the freeze is the retained diagnostic vector itself, not a per-session cache. +34. **Frozen counts stay O(1) at frame cadence** (PR round 5, Rust + unit): `DiagnosticStore` caches all four per-URI severity totals; + replacement updates them, `mark_stale` preserves them, and + `clear` / an empty replacement remove them with the diagnostic + vector. `StatusFacts` reads this tuple rather than rescanning + `for_uri` for every semantic session on every frame. diff --git a/src/diag.rs b/src/diag.rs index 70dece5..c7ea236 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -196,6 +196,10 @@ impl Diagnostic { #[derive(Default)] pub struct DiagnosticStore { by_uri: HashMap>, + /// Per-URI severity totals, maintained alongside `by_uri` so + /// frame-time consumers do not rescan every diagnostic at render + /// cadence. Entries exist exactly when `by_uri` entries do. + severity_counts: HashMap, /// URIs whose stored diagnostics are known to be out of date /// because a `textDocument/didChange` was issued after the last /// `publishDiagnostics` was absorbed. `Self::set` clears entries @@ -210,6 +214,20 @@ pub struct DiagnosticStore { epochs: HashMap, } +fn count_severities(diags: &[Diagnostic]) -> (u32, u32, u32, u32) { + let mut counts = (0u32, 0u32, 0u32, 0u32); + for diagnostic in diags { + let slot = match diagnostic.severity { + DiagnosticSeverity::Error => &mut counts.0, + DiagnosticSeverity::Warning => &mut counts.1, + DiagnosticSeverity::Information => &mut counts.2, + DiagnosticSeverity::Hint => &mut counts.3, + }; + *slot = slot.saturating_add(1); + } + counts +} + impl DiagnosticStore { /// Empty store. #[must_use] @@ -227,11 +245,14 @@ impl DiagnosticStore { pub fn set(&mut self, uri: impl Into, mut diags: Vec) { diags.sort_by(Diagnostic::compare_by_position); let uri = uri.into(); + let counts = count_severities(&diags); self.stale_uris.remove(&uri); *self.epochs.entry(uri.clone()).or_insert(0) += 1; if diags.is_empty() { self.by_uri.remove(&uri); + self.severity_counts.remove(&uri); } else { + self.severity_counts.insert(uri.clone(), counts); self.by_uri.insert(uri, diags); } } @@ -240,6 +261,7 @@ impl DiagnosticStore { /// no entry to be stale about. pub fn clear(&mut self, uri: &str) { self.by_uri.remove(uri); + self.severity_counts.remove(uri); self.stale_uris.remove(uri); *self.epochs.entry(uri.to_owned()).or_insert(0) += 1; } @@ -285,19 +307,26 @@ impl DiagnosticStore { let mut w = 0; let mut i = 0; let mut h = 0; - for diags in self.by_uri.values() { - for d in diags { - match d.severity { - DiagnosticSeverity::Error => e += 1, - DiagnosticSeverity::Warning => w += 1, - DiagnosticSeverity::Information => i += 1, - DiagnosticSeverity::Hint => h += 1, - } - } + for &(errors, warnings, information, hints) in self.severity_counts.values() { + e += errors as usize; + w += warnings as usize; + i += information as usize; + h += hints as usize; } (e, w, i, h) } + /// Per-URI totals `(error, warning, information, hint)`. + /// + /// The tuple is computed once by [`Self::set`] and deliberately + /// survives [`Self::mark_stale`]: staleness invalidates byte + /// positions, while the last published counts remain valid as a + /// frozen status summary until the next publication. + #[must_use] + pub fn severity_counts_for(&self, uri: &str) -> (u32, u32, u32, u32) { + self.severity_counts.get(uri).copied().unwrap_or_default() + } + /// Per-URI count. #[must_use] pub fn count_for(&self, uri: &str) -> usize { @@ -860,12 +889,47 @@ mod tests { assert_eq!(totals, (2, 1, 1, 1)); } + #[test] + fn cached_severity_counts_replace_clear_and_survive_staleness() { + let mut store = DiagnosticStore::new(); + assert_eq!(store.severity_counts_for("file:///a"), (0, 0, 0, 0)); + + store.set( + "file:///a", + vec![ + diag(0, DiagnosticSeverity::Error, "1"), + diag(1, DiagnosticSeverity::Error, "2"), + diag(2, DiagnosticSeverity::Warning, "3"), + diag(3, DiagnosticSeverity::Information, "4"), + diag(4, DiagnosticSeverity::Hint, "5"), + ], + ); + assert_eq!(store.severity_counts_for("file:///a"), (2, 1, 1, 1)); + + store.mark_stale("file:///a"); + assert_eq!( + store.severity_counts_for("file:///a"), + (2, 1, 1, 1), + "staleness freezes the last published counts" + ); + + store.set( + "file:///a", + vec![diag(0, DiagnosticSeverity::Warning, "replacement")], + ); + assert_eq!(store.severity_counts_for("file:///a"), (0, 1, 0, 0)); + + store.clear("file:///a"); + assert_eq!(store.severity_counts_for("file:///a"), (0, 0, 0, 0)); + } + #[test] fn empty_set_clears_uri() { let mut s = DiagnosticStore::new(); s.set("a", vec![diag(0, DiagnosticSeverity::Error, "x")]); s.set("a", Vec::new()); assert!(s.for_uri("a").is_empty()); + assert_eq!(s.severity_counts_for("a"), (0, 0, 0, 0)); assert_eq!(s.uris().count(), 0); } diff --git a/src/semantic_render.rs b/src/semantic_render.rs index d2b4f76..cf64e7a 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -889,12 +889,12 @@ impl SemanticRenderState { /// last published value while the diag store is stale — mid-edit /// positions are wrong but *counts* merely lag, and flickering /// to zero on every keystroke would be worse. The freeze IS the - /// store's retained vector (rounds 3–4): `mark_stale` keeps the - /// last published diagnostics, so counting them while stale - /// yields the frozen value with no session state to lose — not - /// to a snapshot reset, and not by attaching mid-edit. The - /// daemon's write loop keeps the variant off wires negotiated - /// `< 8`. + /// store's retained state (rounds 3–5): `mark_stale` keeps the + /// last published diagnostics and their cached severity totals, + /// so reading the totals while stale yields the frozen value in + /// O(1) with no session state to lose — not to a snapshot reset, + /// and not by attaching mid-edit. The daemon's write loop keeps + /// the variant off wires negotiated `< 8`. fn status_facts_msg( &mut self, state: &EditorState, @@ -917,25 +917,22 @@ impl SemanticRenderState { buffer_file_uri(&core, buffer_id).map_or((0, 0), |uri| { let store = state.lsp_manager.borrow().diag_store(); let guard = store.lock().expect("diag store mutex poisoned"); - // Counted even while the store is STALE (round 4): - // `mark_stale` keeps the last published vector (T - // M11.8) — positions are invalid mid-edit, but counts - // merely lag, so the retained entries ARE the frozen - // value. Sourcing the freeze from the store rather - // than any per-session cache means a session first - // rendering during staleness — a late joiner, or a - // buffer first visited mid-edit — reports the + // Read even while the store is STALE (round 4): + // `mark_stale` keeps the last published diagnostics + // (T M11.8) — positions are invalid mid-edit, but + // counts merely lag, so the retained totals ARE the + // frozen value. Sourcing the freeze from the store + // rather than any per-session cache means a session + // first rendering during staleness — a late joiner, + // or a buffer first visited mid-edit — reports the // preserved counts instead of zeros, and the snapshot // reset has nothing count-related to preserve. - let mut errors = 0u32; - let mut warnings = 0u32; - for d in guard.for_uri(&uri) { - match d.severity { - crate::diag::DiagnosticSeverity::Error => errors += 1, - crate::diag::DiagnosticSeverity::Warning => warnings += 1, - _ => {} - } - } + // `DiagnosticStore::set` computes this tuple once + // (round 5). StatusFacts runs at frame cadence for + // every semantic session, so rescanning the retained + // vector here would make stale intervals + // O(frames * diagnostics). + let (errors, warnings, _, _) = guard.severity_counts_for(&uri); (errors, warnings) }) }; diff --git a/tests/theme_faces_acceptance.rs b/tests/theme_faces_acceptance.rs index 4b6a3da..c591839 100644 --- a/tests/theme_faces_acceptance.rs +++ b/tests/theme_faces_acceptance.rs @@ -1,7 +1,8 @@ // theme_faces_acceptance.rs --- Themes Arc 4 stage 1 acceptance // (docs/theme-faces-framing.md, acceptance items 1–19, 24–26, 28–29, // and 32–33; the GPU routes — 20–23, 27, and 30–31 — live in -// pmacs-gpu's headless suite). +// pmacs-gpu's headless suite; item 34 is a `DiagnosticStore` unit in +// src/diag.rs). //! Named UI faces (`ui` / `ui.*` theme entries) + the `ThemeFacts` //! wire channel (protocol v16).