From ac6fbaf9f37b75b985aa96412bbc7cf639127984 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] 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>
---
 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 7273ba38..9deea628 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 273cf90d..ac039f6a 100644
--- a/cmd/server/store.go
+++ b/cmd/server/store.go
@@ -478,6 +478,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.
@@ -4536,13 +4543,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.