Skip to content

port(upstream#1873): reuse ctx buffer in resolvePathForObs, cache ReadMemStats - #39

Merged
dborup merged 3 commits into
masterfrom
codex/port-upstream-1873-ctx-buffer-memstats-cache
Sep 20, 2026
Merged

port(upstream#1873): reuse ctx buffer in resolvePathForObs, cache ReadMemStats#39
dborup merged 3 commits into
masterfrom
codex/port-upstream-1873-ctx-buffer-memstats-cache

Conversation

@adminopenclaw8-sketch

@adminopenclaw8-sketch adminopenclaw8-sketch commented Sep 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Split out of #25 (commit 9ef31367 there). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.

Upstream

Problem

resolvePathForObs allocated and copied a fresh context slice for every hop. estimatedMemoryMB called runtime.ReadMemStats (stop-the-world) on every stats/debug call.

Change

resolvePathForObs reuses one context buffer, trimmed back to the base context per hop. estimatedMemoryMB caches the heap reading per store for 5 s behind a mutex; the injectable memoryEstimator used by tests is unchanged.

Adaptation to this fork

None. The cherry-pick applied without conflicts and the changed lines are identical to upstream.

Notes for review

AGENTS.md rule 0 (perf proof): the upstream PR carries no benchmark or profile, and this split does not add one. Reviewers should treat the perf benefit as unproven. The observable side effect is that memory figures in stats/debug endpoints can be up to 5 s old.

Dependencies and merge order

Verification

Local run of the same commands as CI's “Go Build & Test” job (server tests with -race), on this branch and on master fda24ca5 under the same conditions (same machine, run one after another):

Check master fda24ca5 this branch verdict
go-server-build-vet PASS PASS
go-server-test-race PASS PASS
channel-lib-test PASS PASS
decrypt-cli-build-test PASS PASS
dockerfile-copy-invariants FAIL FAIL not runnable locally: script needs bash ≥4 (declare -A), macOS has 3.2; identical on master
staging-disk-monitor PASS PASS
css-vars-lint PASS PASS

Baseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.

Browser validation (local, fixture DB, no staging/production): Not applicable (no frontend change).

Not run:

  • Playwright E2E suites (no local Playwright install); CI's E2E job will also be skipped, see below.
  • eslint (not installed locally; CI installs it on the fly).
  • Frontend JS suites (no frontend change).
  • Browser tests against staging/production (deliberately none).

Expected GitHub CI: “Go Build & Test” is expected to fail on TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run on push to master and cannot run for this PR.
Two further ingestor tests have failed intermittently in this split's CI on branches whose cmd/ingestor tree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:

GitHub CI result: run 34751048597 on 3028e8a0. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:

  • TestPruneOldNeighborMetrics: fails on master, documented baseline
  • TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: intermittent, reproduced on unmodified master locally

🤖 Generated with Claude Code

Joel-Claw and others added 3 commits September 13, 2026 10:42
@Joel-Claw Joel Claw
@claude Openclaw
perf: reuse ctx buffer in resolvePathForObs, cache ReadMemStats per s…
3028e8a 
…tore (Kpa-clawbot#1873)

## 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 ac6fbaf)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dennis Jakobsen @claude Dennis Jakobsen
Merge origin/master into PR #39
ed58d81 
Brings the branch up to master 834c8da (post #35, which also touched
cmd/server/store.go in a different region) so CI runs on current master.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dennis Jakobsen @claude Dennis Jakobsen
reviewfix(perf): correct two inaccurate comments
55e0dc9 
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 <noreply@anthropic.com>
@dborup

dborup commented Sep 20, 2026

Copy link
Copy Markdown
Owner

Uafhængig review + målt verifikation

Gennemgået af en reviewer, der ikke skrev ændringen. Ingen blockers. Begge risikoområder, jeg flaggede i beskrivelsen (aliasing og kapacitet), kom rene tilbage — men perf-påstanden er nu målt, og den er væsentligt mindre end porten antyder. AGENTS.md regel 0 er hermed opfyldt med tal.

1. Aliasing-sikkerhed — ren (tracet + testet)

resolveWithContext har ingen sti, der gemmer, appender til eller sorterer contextPubkeys/lowerCtx:

  • Tier 1 (store.go:6965-6979): lowerCtx := contextPubkeys aliaser, men bruges kun som for _, ctxPK := range lowerCtx → graph.Neighbors(ctxPK), der tager en string.
  • Tier 2 geo (store.go:7043): kun læsning (strings.ToLower + map-opslag).
  • Tier 3/4: rører den ikke.
  • Den eneste in-place-sortering i funktionen er over en lokalt bygget []scored, ikke context-slicen.

Reviewerens tests (alle grønne, derefter fjernet igen — worktree er ren):

  • Sentinel/mutation: forgiftede den ledige kapacitet, snapshottede indhold + len/cap før hvert kald og krævede dem uændrede bagefter.
  • Retention: overskrev hele backing-arrayet + runtime.GC() efter hvert kald og sammenlignede med en kørsel med friske slices — identisk, intet tilbageholdes.
  • Differential: 300 tilfældige fixtures × 6 context-former (begge / kun observer / kun fromNode / ingen / uppercase-observer / nil-graph) mod en ordret kopi af koden før PR'en — identiske resultater.
  • Concurrency: 32 goroutines × 200 iterationer mod seriel kørsel — identisk.

Testene er følsomme: en fejl i reviewerens eget harness fik dem til at fejle højlydt med et tier-skift (neighbor_affinity → geo_proximity), så de fanger rent faktisk context-korruption.

Concurrency er i øvrigt et ikke-spørgsmål: begge produktionskaldere holder den eksklusive store-lås (store.go:2759 dækker store.go:2854; store.go:3140 dækker store.go:3199), og ctx er funktionslokal uanset.

2. Kapacitet/reset — ren (tracet + målt)

contextPKs er make([]string, 0, 3) med højst 2 appends. ctx nulstilles til ctx[:ctxLen] og får højst ét append → len ≤ ctxLen+1 ≤ cap. Ingen sti kan overskride kapaciteten, og trunkeringen før append gør et forældet element umuligt.

Målt bevis for nul reallokering: de sparede allokeringer er præcis nHops-1 ved hver størrelse (hops 1/4/8/16/32 → ny 22/33/46/71/120 mod gammel 22/36/53/86/151). En skjult reallokering ville reducere den differens.

3. Målinger (Apple M2 Pro, -benchtime=3000x -count=6, medianer)

Ændring A, ende-til-ende resolvePathForObs:

scenarie før efter delta allocs B/op
3 hops, 1 kandidat 1381 ns 1357 ns −1,8 % (støj) 28→26 1488→1360
8 hops, 3 kandidater 4702 ns 4649 ns −1,1 % (støj) 53→46 2528→2080
32 hops, 4 kandidater 16081 ns 15277 ns −5,0 % (ren adskillelse) 151→120 7248→5264

Isoleret buffer-arbejde alene: 8 hops 213→45 ns (−79 %), 32 hops 846→64 ns (−92 %). Selve optimeringen er 5-13× hurtigere, men udgør kun ~1-4 % af funktionens samlede omkostning — resolveWithContext' egne per-kald-allokeringer (candPKs/bestScores/bestCounts/candByPK) dominerer. Så: 1-2 %, ikke en størrelsesorden. Allokeringsreduktionen er derimod reel og deterministisk (nHops-1 × 64 B) og er ægte GC-pres-lettelse.

Ændring B: ReadMemStats 22.246 ns/op (og 21.832-22.529 ns med 256 MB levende heap — omkostningen er STW-domineret, ikke heap-størrelse-domineret). Cachet: 44 ns/op, altså ~500× per kald, 0 allokeringer begge veje. Parallelt over 12 procs: 23.176 → 190 ns.

4. SHOULD-FIX, som jeg bevidst ikke retter her (og hvorfor)

Disse er reelle, men at rette dem ville ændre porten væk fra upstream ac6fbaf9 og dermed koste paritet for fremtidige ports. Jeg dokumenterer dem i stedet:

  • (a) Cachen i ændring B kan aldrig ramme på /api/stats. routes.go:981-984 cacher allerede hele /api/stats-svaret i statsTTL = 10s bag en singleflight. 10 s > 5 s, så den indre cache er uopnåelig ad den vej. (tracet)
  • (b) På /api/perf er TTL == poll-interval, så hit-raten er nul. public/perf.js:376 poller hvert 5000. ms mod en 5 s TTL. Målt: 0 hits / 4 polls ved reelt 5 s mellemrum. Gevinsten opstår først ved ≥2 samtidige perf-seere. (målt)
  • (c) Forældelsen lander præcis der, hvor gevinsten påstås. Målt: efter allokering af ~200 MB blev det rapporterede tal stående på 130,1 MB. Et heap-spring er usynligt i op til 5 s på netop den admin-side, man åbner for at se hukommelse. Worst case svarer dog til ét poll-interval, og adfærden er allerede dokumenteret i PR-beskrivelsen. (målt)
  • (d) ctx-kopien i ændring A er død vægt. contextPKs er funktionslokal, læses aldrig efter løkken og har altid ≥1 ledig plads, så ctx := contextPKs ville fjerne den sidste allokering (3 hops: 1336→1269 ns, 26→25 allocs; 8 hops: 4838→4685 ns). Det er en forbedring ud over upstream og hører til en selvstændig PR, ikke i en port. (målt)

Konklusion på (a)-(c): ændring B er i praksis næsten en no-op — ingen målbar gevinst for én operatør, marginal for flere, og prisen er ét poll-intervals forældelse på perf-siden. Den er ikke skadelig, men den er heller ikke den optimering, upstream-titlen lover. Ændring A's allokeringsreduktion er derimod ægte og deterministisk. Samlet vurdering: korrekt og sikker kode, beskeden reel værdi — merges for upstream-paritet, ikke for perf-gevinsten.

5. Rettet i 55e0dc91 (kun kommentarer)

To kommentarer, jeg selv skrev i porten, påstod noget usandt:

  • neighbor_persist.go: kommentaren sagde "+2 for the previous-hop resolved PK" (ental), men reserverede 2. Kun ét element appendes nogensinde. Omformuleret til den faktiske invariant.
  • store.go: lock-ordering-kontrakten øverst i PacketStore siger "When adding new lock acquisitions, respect this ordering", men estMemMu stod ikke på listen, og Load() nester nu mu → estMemMu. Tilføjet som streng blad (7) og nestingen noteret.

Ingen adfærdsændring. (Kontrakten er i forvejen forældet for statsCacheMu, bgErrMu, chunkCBMu og analyticsRecomputerMu — det ligger uden for denne PR's scope.)

6. Kontrolleret og fundet i orden

  • Per-store-cachen er ægte: to &PacketStore{}-instanser har uafhængig tilstand (målt: forældet a=2,7 MB mod frisk b=130,1 MB). Nulværdi-store læser friskt ved første kald (time.Since(time.Time{}) er enorm — ingen stale-0.0-fejl). memoryEstimator-injektionen omgår fortsat cachen helt.
  • Ingen test afhænger af en frisk aflæsning: routes_test.go:187 tjekker kun nøglers tilstedeværelse, og testdata/golden/shapes.json:923,1439 kræver kun "type": "number".
  • At holde estMemMu hen over de 22 µs STW i ReadMemStats er sikkert og strengt bedre end N samtidige STW-pauser.
  • go build, go vet (inkl. copylocks for den nye sync.Mutex i PacketStore) og hele go test ./... grønne.
  • Mit eget fulde go test -race ./... på merge-head ed58d815: grønt, 245 s, 0 fejl, 0 races.

7. Sync og CI

Master merget ind som almindelig merge-commit ed58d815 (parents 3028e8a0 + 834c8da9) — ingen rebase, squash eller force-push. Verificeret at #35's retainResolvedPathHops stadig er intakt efter merge. Frisk CI kører på 55e0dc91.

Staging vurderes ikke nødvendig: ændringen er ren server-intern mikro-optimering uden UI-, database-, MQTT- eller netværksadfærd, og den er fuldt dækket af -race-suiten plus benchmarks ovenfor. Den eneste observerbare effekt (op til 5 s gamle hukommelsestal i stats/perf) er målt direkte her.

🤖 Generated with Claude Code

@dborup
dborup merged commit 16ffe10 into master Sep 20, 2026
6 checks passed
This was referenced Sep 20, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adminopenclaw8-sketch @dborup @Joel-Claw