From 3028e8a044e13ae921e7fd39a730e651fa96f82e Mon Sep 17 00:00:00 2001 From: Joel Claw <274497724+Joel-Claw@users.noreply.github.com> Date: Wed, 2 Sep 2026 23:20:39 +0200 Subject: [PATCH 1/2] perf: reuse ctx buffer in resolvePathForObs, cache ReadMemStats per store (#1873) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Three hot-path inefficiencies causing excess CPU and memory allocations: ### 1. `filterTxSlice` starts with nil slice `filterTxSlice` is called on the full `s.packets` slice (50k+ packets) for every query that doesn't hit a fast-path index. Starting with `var result []*StoreTx` means Go's append does ~15 growth+copy cycles (1→2→4→8→...→32768→65536) before reaching steady state. ### 2. `resolvePathForObs` allocates per hop Each hop in the path resolution loop allocates a new `ctx` slice (`make([]string, len(contextPKs), len(contextPKs)+2)`). For a 5-hop path, that's 5 allocations per observation. With 500+ observations per ingest batch, that's 2500+ small allocations. ### 3. `estimatedMemoryMB` calls `runtime.ReadMemStats` without caching `runtime.ReadMemStats()` triggers a STW (stop-the-world) pause. It's called from stats/debug endpoints (`GetStoreStats`, `GetPerfStoreStats`) that may be polled frequently. The routes.go layer already caches this with a 5s TTL, but the store layer doesn't. ## Fix 1. **Pre-allocate `filterTxSlice`**: `make([]*StoreTx, 0, n/2)` — the 2x over-allocation is cheaper than repeated growth+copy. 2. **Reuse ctx buffer**: Allocate one `ctx` buffer before the hop loop, reset to base length each iteration with `ctx = ctx[:ctxLen]`. 3. **Cache `ReadMemStats`**: 5-second TTL cache matching the routes.go pattern. Uses a package-level mutex (not on `PacketStore`) to avoid adding a field. ## Testing - `go build` passes - No behavior change — same results, fewer allocations --------- Co-authored-by: Joel Claw <358739783+Joel-Claw@users.noreply.github.com> (cherry picked from commit ac6fbaf9f37b75b985aa96412bbc7cf639127984) Co-Authored-By: Claude Opus 5 --- cmd/server/neighbor_persist.go | 9 +++++++-- cmd/server/store.go | 24 +++++++++++++++++++++--- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/cmd/server/neighbor_persist.go b/cmd/server/neighbor_persist.go index 5204147be..134b6b530 100644 --- a/cmd/server/neighbor_persist.go +++ b/cmd/server/neighbor_persist.go @@ -132,9 +132,14 @@ func resolvePathForObs(pathJSON, observerID string, tx *StoreTx, pm *prefixMap, contextPKs = append(contextPKs, strings.ToLower(fromNode)) } resolved := make([]*string, len(hops)) + // Reuse a single ctx buffer across hops instead of allocating per hop. + // Capacity +2 for the previous-hop resolved PK that may be appended. + ctx := make([]string, len(contextPKs), len(contextPKs)+2) + copy(ctx, contextPKs) + ctxLen := len(ctx) for i, hop := range hops { - ctx := make([]string, len(contextPKs), len(contextPKs)+2) - copy(ctx, contextPKs) + // Reset to base context (trim any previously appended resolved PK) + ctx = ctx[:ctxLen] if i > 0 && resolved[i-1] != nil { ctx = append(ctx, *resolved[i-1]) } diff --git a/cmd/server/store.go b/cmd/server/store.go index 7a4a6ba86..5ba8f06d5 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -476,6 +476,13 @@ type PacketStore struct { trackedBytes int64 // running total of estimated packet store memory memoryEstimator func() float64 // injectable for tests; nil = use runtime.ReadMemStats (stats only) + // Per-store ReadMemStats cache (5s TTL). Fields (not package-level vars) so + // that test helpers constructing &PacketStore{...} directly get independent + // cache state, avoiding order-dependent test failures. + estMemMu sync.Mutex + estMemVal float64 + estMemAt time.Time + // Short-lived cache for the observations aggregate in GetStoreStats (30s TTL). // Avoids a per-/api/stats full-table scan; values accurate to ~30s which is // sufficient for dashboard display. @@ -4587,13 +4594,24 @@ func estimateStoreObsBytes(obs *StoreObs) int64 { // estimatedMemoryMB returns current Go heap allocation in MB. // Kept for stats/debug endpoints only — NOT used in eviction decisions. // In tests, memoryEstimator can be set to inject a deterministic value. +// Caches the result for 5 seconds because runtime.ReadMemStats() stops the +// world and this is called from stats/debug endpoints that may be polled. +// The cache is per-store (not package-level) so that test helpers constructing +// &PacketStore{...} directly get independent cache state, avoiding +// order-dependent test failures from a shared global cache. func (s *PacketStore) estimatedMemoryMB() float64 { if s.memoryEstimator != nil { return s.memoryEstimator() } - var ms runtime.MemStats - runtime.ReadMemStats(&ms) - return float64(ms.HeapAlloc) / 1048576.0 + s.estMemMu.Lock() + defer s.estMemMu.Unlock() + if time.Since(s.estMemAt) > 5*time.Second { + var ms runtime.MemStats + runtime.ReadMemStats(&ms) + s.estMemVal = float64(ms.HeapAlloc) / 1048576.0 + s.estMemAt = time.Now() + } + return s.estMemVal } // trackedMemoryMB returns the self-accounted packet store memory in MB. From 55e0dc91e781b9a36b8ad7899b4f60ac31b8abd5 Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Sun, 20 Sep 2026 21:15:29 +0200 Subject: [PATCH 2/2] reviewfix(perf): correct two inaccurate comments Independent review found no blocker in either optimisation, but two comments I wrote in the port assert things that are not true: - neighbor_persist.go: the comment claimed the spare capacity was "+2 for the previous-hop resolved PK", but at most one element is ever appended. Reworded to state the actual invariant (one append, spare capacity is headroom so the append never reallocates). - store.go: the lock-ordering contract at the top of PacketStore says "When adding new lock acquisitions, respect this ordering", but the new estMemMu was not listed, and Load() now nests mu -> estMemMu. Added estMemMu as a strict leaf (7) and recorded the nesting. Comment-only; no behaviour change. Co-Authored-By: Claude Opus 5 --- cmd/server/neighbor_persist.go | 3 ++- cmd/server/store.go | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/server/neighbor_persist.go b/cmd/server/neighbor_persist.go index 134b6b530..ccc6f5d76 100644 --- a/cmd/server/neighbor_persist.go +++ b/cmd/server/neighbor_persist.go @@ -133,7 +133,8 @@ func resolvePathForObs(pathJSON, observerID string, tx *StoreTx, pm *prefixMap, } resolved := make([]*string, len(hops)) // Reuse a single ctx buffer across hops instead of allocating per hop. - // Capacity +2 for the previous-hop resolved PK that may be appended. + // At most one element (the previous hop's resolved PK) is ever appended; + // the spare capacity is headroom so that append never reallocates. ctx := make([]string, len(contextPKs), len(contextPKs)+2) copy(ctx, contextPKs) ctxLen := len(ctx) diff --git a/cmd/server/store.go b/cmd/server/store.go index ed32541c0..b801a36ec 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -156,12 +156,17 @@ func (tx *StoreTx) ParsedDecoded() map[string]interface{} { // result (hashSizeInfoCache). Acquired independently or // under mu (in EvictStale). // +// 7. estMemMu (sync.Mutex) — guards the ReadMemStats result cache +// (estMemVal/estMemAt). Strict leaf: no other lock is +// acquired while holding it. +// // Nesting that occurs today: // - IngestNew: mu → cacheMu → channelsCacheMu (1 → 2 → 3, OK) // - IngestObservations: mu → cacheMu (1 → 2, OK) // - RunEviction/EvictStale: mu → cacheMu → channelsCacheMu (1 → 2 → 3, OK) // - RunEviction/EvictStale: mu → hashSizeInfoMu (1 → 6, OK) // - invalidateCachesFor: cacheMu → channelsCacheMu (2 → 3, OK) +// - Load (startup log lines): mu → estMemMu (1 → 7, OK) // // All other locks are acquired independently (no nesting). // When adding new lock acquisitions, respect this ordering.