Repository navigation
perf(store): on-demand subpath counts instead of permanent indexes - #529
Conversation
… alone (#413) Equivalence of /subpaths, /subpaths-bulk, windowed, region and subpath-detail results with a frozen copy of the spIndex/spTxIndex the store keeps today, through ingest, best-path changes and eviction on synthetic 30K-tx-shaped data; totalPaths after a real eviction; no permanent index and no O(hops^2) memory charge; bounded top-N ranking vs full sort with ties at the cut; counting outside PacketStore.mu; and a -race test with concurrent writers. Red on master: the endpoints read an index the packets do not feed, eviction leaves spTotalPaths behind, the store still holds the index and charges it, and the counting runs under the read lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…TxIndex (#413) The store kept every 2-8 hop subpath of every transmission in two permanent maps, updated on ingest, best-path change, hash merge and eviction and rebuilt under the write lock at load. On 30K transmissions with the fixture's path lengths that is 414K keys / 628K tx refs, ~51 MB of heap, ~7 us per ingested and ~15 us per evicted transmission, and a ~325 ms rebuild. Route Patterns now count from the packets per request: - only the hop lists (shared, never edited in place) and the aggregate hop context are copied under PacketStore.mu; enumerating, resolving and ranking run after it is released (subpath-detail copies the few fields it reads of the matched transmissions); - the unfiltered list keeps the index's semantics exactly: lower-cased comma-joined raw keys, 2 to 8 hops, every occurrence counted, rawHops the smallest raw key behind a name; window and region queries keep their per-transmission semantics; - rankSubpaths keeps the top N in a bounded heap with the full (count, path) order, so ties break on the path before the cut, as the sort did; - totalPaths is the held transmissions with 2+ hops; the index's eviction never decremented it, so pct drifted on a long-running server. The subpath ready gate and its 503 go (nothing to build); the path-hop gate and its test hook stay. The memory budget no longer charges O(hops^2) per transmission for the index. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent2 PR#529 #413 — head cb8ad36Status: Draft, every CI job green on the first run. All requirement tests are red before and green after, except two preservation guards that are already green on master (see below). 18 of 18 mutants are killed. Not merged, not marked ready. Evidence tags: [T] = test or measurement run, output observed. [A] = analysis or derivation from measured data. [K] = verified by reading code or diff. Requirements
The commit sequence follows the rows above. Test commit Invariants
Local runs
CI per job (run 39970068648, head cb8ad36)
No reruns were needed, and the known flaky test #271 did not fail [T]. Remaining items
|
Review — CS-Macmini PR#529 — head cb8ad36Dom: REQUEST CHANGES Independent, read-only review. The core of the change holds up. I reproduced the equivalence claim far past the author's 52 responses, and the list-side lock win is real and large. Two things need a decision before this merges: the per-request full scan has no concurrency bound, and the combination with #527 turns a test red deterministically. Findings
F1 — the full scan per request has no concurrency boundAll on
Peak live heap during N concurrent cold requests for the same uncached key, 3 runs each [T]:
Per-request allocation is flat in N in both trees (166 MB and 257 MB respectively), so neither dedupes [T]. The read: the PR is clearly better at one request in flight, about even at four, and ~90–130 MB worse at eight — the resident saving is spent on transient per-request maps as soon as a few misses overlap [A]. This is not a regression in kind: master stampedes identically, and F2 — the detail's lock hold now scales with the store
Master's hold is O(matches); this one is O(N_packets), so it grows with the store rather than with the answer. The list direction is the opposite and is the big win — worst writer wait during one cold list request: 267 ms over 1 acquisition on master (a writer was blocked for the whole request) → 26.5 ms over 172 acquisitions here [T]. Two related observations under that same RLock [K]: F3 — the #527 combination is deterministically red
After resolving those comments, though, Cause [T][A]: the test sets
Merge order does not help: the arithmetic depends on the combined estimate, so whichever lands second turns master red. The test's own comment ("one pass needs about 15-24% of the store, under the 25% safety cap") states the assumption that breaks, and master is already at 22.65 %, so the right fix is to derive Answers to the review pointsEquivalence. Reproduced and extended, byte-identically, two ways.
Note on deliberate difference 3 (float summation order in Latency, cold and warm, and the stampede. Table and peak-heap numbers in F1. Warm is a The #495 lock rule. Copy under the lock, count outside, is implemented as described for the list, bulk, window and region paths [K], and the measured writer wait drops 267 ms → 26.5 ms [T]. The detail is the exception, see F2.
#527 overlap. See F3. Tests and mutantsEverything below on the merge tree against current master
Red before, green after. Verified independently on the test-only commit My own mutants. Two, both in
M-A is N1: hops are hex prefixes in practice so the real exposure is nil, but it is new code with no test. M-B is N2: Invariants
The Not verified
|
Brings in #527 (#411). Conflicts were comment-only: the Kpa-clawbot#743 list and the estimateStoreTxBytes doc comment in store.go, and the manual-calc comment in eviction_test.go. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… cap (#413) TestMemoryEviction_CrossesHighAndReachesLow_113 set maxMemoryMB to the whole MiB below the tracked size T, so one pass must free 1-0.85*floor(T)/T of the store. With #411 and #413 both removing a per-tx term from the estimate, the 3000-tx store tracks 7.94 MiB and the share is 25.03%, over the 25% per-pass safety cap: the pass stops 5.6 KB above the low watermark. obsIndexBytes plays no part (the fixture has at most 4 observations per tx, below obsIndexThreshold). The store is now sized from a first load to track at least 16 MiB, where the share stays between 15% and 20.4% whatever the per-tx estimate, and the setup fails loudly if the share comes within 4 points of the cap. The assertions are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…413) An uncached Route Patterns request scans the whole store, and N concurrent misses for one key each ran that count (PR #529 review F1). The misses of GetAnalyticsSubpaths, GetAnalyticsSubpathsWithWindow and the unfiltered GetAnalyticsSubpathsBulk now go through one singleflight.Group, keyed on the cache key (bulk: its groups' keys), the statsSF pattern: the flight checks subpathCache again first, so a caller that missed just before an earlier count stored its result does not count again. TestSubpathColdMissesShareOneCount_413 holds the first count until all 8 callers have missed (through the new test-only subpathHook) and wants exactly 1 count for list, window and bulk; with the flight bypassed it gets 8, 3 runs of 3. BenchmarkSubpathsColdConcurrent_413, 30K tx, peak heap of 3 runs: n=1: 140-146 MB -> 131-141 MB (1 count either way) n=4: 441-482 MB -> 131-148 MB (4 counts -> 1) n=8: 907-914 MB -> 138-147 MB (8 counts -> 1) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SubpathDetail compared every packet's path under PacketStore.mu, so a writer's wait grew with the store, not with the answer (PR #529 review F2). Under the lock it now only copies each transmission's pointer and hop list into a slice allocated beforehand; matching runs after the unlock, and a second RLock copies what the detail reads of the matched transmissions, O(matches). A transmission's per-tx hop context is built once instead of once per occurrence. TestSubpathCountingReleasesStoreLock_413 gets a subpath-detail-match case that parks the request inside matching (subpathHook "match") and wants a writer to take the lock; red with matching under the lock. BenchmarkSubpathDetailWriterWait_413, 30K tx, GC off, worst writer wait over 50 requests, 3 runs: hot pair (98 matches): 2527/3279/2674 us -> 1231/1202/732 us cold 5-hop (1 match): 1834/2201/1831 us -> 641/471/795 us Request latency is unchanged (hot 2.8 ms, cold 1.8 ms). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…th counts (#413) PR #529 review N1/N2. TestSubpathNonASCIIHopsFoldCase_413 checks appendLowerHop and matchLowerHop against strings.ToLower for non-ASCII hops (including "ẞ", which folds one byte shorter) and that list and detail merge differently-cased hops; killed: the utf8.RuneSelf fallback removed from both. TestSubpathsBulkGroupOrderDoesNotCapLength_413 wants every bulk group equal to the same group on its own for group orders that put the longest group first or in the middle; killed: maxLen = g.MaxLen in countSubpathGroups. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…413) The singleflight helpers spelled the result type out eight times; they now hand around the *cachedResult that subpathCache stores, so map[string]interface{} in non-test cmd/server code goes 733 -> 732 instead of 741. Ranking stays outside cacheMu. No behaviour change: the same tests and the no-flight mutant (8 counts) as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent1 PR#529 runde-2 — head b3ab527Review feedback addressed (commit Evidence tags: [T] run and observed, [A] derived from measured data, [K] read in the code. All timings are from Go 1.27.1, linux/amd64, a 4-vCPU VM, on
Tests
Pre-existing flaky test (not #271 or #301).
It touches no subpath code [K]. It did not fail in CI. I have not filed an issue for it. Mutants (round 2): 6 run, 6 killed
CI per job: run 40060602106, head
|
| Job | Result |
|---|---|
| Go Build & Test | pass |
| Go Server Tests (1/3), (2/3), (3/3) | pass, pass, pass |
| Go Ingestor Tests | pass |
| Go Libraries & CLI | pass |
| Verify Go Test Assignments | pass |
| JavaScript Unit Tests & Lint | pass |
| Script & Container Unit Checks | pass |
| Playwright E2E Tests (1/3), (2/3), (3/3) | pass, pass, pass |
| Verify E2E Shards & Coverage | pass |
| Validate Docker Image | pass |
| Build & Publish Docker Image | pass |
| Release Artifacts, Publish Badges & Summary, Deploy Staging | skipped |
15 passed, 3 skipped, 0 failed. Neither #271 nor #301 appeared [T].
Commits this round
004a5644 merge origin/master · 0aa39c41 F3 · dfdec4ea F1 · 17d8401e F2 · 5555886e N1/N2 · f4b81ed1 N4 · b3ab5278 F1 refactor (no new map[string]interface{}). All are authored by dborup <kontakt@meshview.dk>. There was no rebase, amend or force-push. The PR stays a draft.
Not verified
- Anything above 30,000 transmissions.
- Real multi-client HTTP load: the concurrency numbers come from in-process goroutines.
- The startup window in N3 was read in the code, not exercised.
- No staging or production access.
- There was no browser check beyond the three E2E files; no frontend file changed.
Review — CS-Macmini PR#529 — head b3ab527Dom: APPROVE med nits Independent, read-only re-review of round 2 against my round-1 review of All three findings are resolved, and the two measurements I pushed back on in round 1 now come out better than master rather than merely better than the round-1 head. The remaining items are test gaps and one documentation number. Findings
F1 — concurrent cold missesCorrectness. I went through the four things worth worrying about:
Peak heap, reproduced. My own probe, not the author's benchmark: the identical Go file compiled in both trees, differing only by a one-line prepare that calls
Peak is flat in N, 5.1–5.8× below master at N=8, and the N=8 crossover I measured in round 1 is gone [A]. Wall time at N=8 drops from 587–607 ms (master) to 247–256 ms [T], and at-rest Tests. F2 — the detail's lock holdI accept the partial disagreement. The first hold is indeed still O(N) pointer-and-slice-header copies rather than O(matches) [K], but the measured consequence is that the endpoint is now better than master in the worst case, and the author's reason for stopping there (taking candidates from Worst
Read: for a key with matches the hold is 2.4–2.5× shorter than master at every size, because master held one RLock across the whole request including the per-occurrence context build, whereas this one holds it for a pointer copy. Head's curve is ~9 ns per transmission, which confirms the author's "about 8 ns/tx" figure [A]. The regression is confined to rare keys, where master's Three things I checked in the refactor itself:
Red before, green after. With the first F3 — the #113 watermark testThe explanation holds; I reproduced every number. On this head, 3000 It is not The fix does not weaken the test. Every assertion after the setup is byte-identical to master's: crossing the high watermark ( And the 25 % cap keeps its own coverage. The helper is also robust to the estimate moving in either direction: it rescales n from a first load and Fatals after three attempts, so a much larger per-tx estimate keeps n at 3000 and a much smaller one scales n up [K]. The test now loads 6,351 transmissions tracking 16.81 MiB, with a required share of 19.09 % [T]. N1, N2 — my round-1 mutantsBoth now die, and specifically:
New nits in the round-2 deltaR1 — the R2 — the in-flight cache re-check has no test. Disabling the R3 — the detail now spans two lock holds. Which transmissions matched, and on which hop list, is as of the first RLock; R4 — the new setup guard cannot fire. With R5 — residual, the detail hold is now O(N_tx) for every key. Numbers in the F2 table. At 200K transmissions a 1-match detail request holds the store lock for 1.83 ms where master held it for 12–64 µs. The worst case is still better than master's and the figure is 14× under the list path's hold, so this is not a merge blocker — it is the shape of the trade, and the Equivalence, re-checked for the round-2 deltaThe delta rewrites
The live fixture has no configured regions, so the HTTP pass did not exercise region filtering this round; the region path is covered by the repo's own Tests and mutantsAll on
My own mutants this round: 6 run, 4 killed, 2 survive as test gaps.
I also re-ran my two round-1 survivors, M-A and M-B; both are now killed, as recorded under N1/N2 [T]. Invariants
Not verified
|
Relates to #413
Summary
Route Patterns (
/api/analytics/subpaths,/subpaths-bulk,/subpath-detail) no longer read the two permanent indexesspIndex/spTxIndex. They count subpaths froms.packetsper request. Only the hop lists are copied underPacketStore.mu; enumerating, resolving and ranking run after the lock is released.The index cost 51 MB of heap per 30K transmissions, about 6 µs per ingested and 13 µs per evicted transmission, and a 285 ms rebuild under the write lock. All of that is gone. Endpoint latency stays the same for the list and goes up by about 2 ms for the detail. Counts, ranking, tie-breaks, window semantics and the API shape are unchanged: on the e2e fixture, 52 of 52 API responses are byte-identical to master.
Plan (milestones)
0350322e, using synthetic data with 30,000 tx and 2,000 nodes. Path lengths follow the hop-count histogram oftest-fixtures/e2e-fixture.db(median 7 hops). Hops are upper-case 1- and 2-byte prefixes, laid out as random walks over a repeater mesh. Measured: index size, per-ingest and per-eviction maintenance, rebuild time, and endpoint latency.fd61b924). A frozen copy of the current index is the oracle, plus lock, eviction, memory and ranking tests. This commit is red on master.cb8ad366). Remove the index and its maintenance, count on demand, rank with a bounded heap, drop the subpath ready gate, and update the memory estimate.-race, mutants,test-all.sh, frontend helpers, E2E against a local server on the CI-prepared fixture, an API diff against a master binary, and a browser check.No config or customizer implications: nothing new is configurable. The 8-hop cap is the index's existing cap, now the named constant
subpathMaxHops.Design choice: incremental vs on-demand
spIndex+spTxIndex)len(spTxIndex[key])On-demand doesn't make the endpoints slower (numbers below), so the bigger win was chosen and no new response cache was needed. The existing bounded
subpathCache(#370: 256 entries, 60 s TTL) still covers repeated renders.Readers of the indexes, and what replaced them
/subpaths, unfiltered (computeAnalyticsSubpaths)spIndexunder RLockcountSubpathGroups: snapshot under RLock, then count outside it/subpaths-bulk, unfilteredspIndexunder RLock/subpathswith region or windowcomputeSubpathsSlow/computeAnalyticsSubpathsWindowed, duplicates of each other)/subpath-detailspTxIndex[key]lookup, everything under RLockreindexTxPath, eviction, hash mergeLoad, background chunk loadbuildSubpathIndexSubpathIndexReadygate + 503pathHopBuildGate) stayestimateStoreTxBytes)spTxIndexThe neighbour graph, other analytics and
/api/nodes/{pk}/pathsnever read these indexes. They usebyPathHop, which is untouched.Contract: what is preserved
rawHopsis the smallest raw key behind a shared name.rawHopsand every length up tomaxLen. The difference from the unfiltered list existed before this PR and is kept as-is.rankSubpathskeeps the top N in a bounded heap ordered by (count desc, path asc). Paths are distinct map keys, so the order is total: the heap selects exactly what the full sort cut at N selected, ties are broken on the path before the cut, and the result is the same on every run (flaky: #226 Hash Stats adopters sort E2E — server sorts multiByteNodes on packets only, ties come out in random order #256).totalMatches(one per occurrence), the same 8-hop cap, and case-insensitive matching.Deliberate differences
totalPathsafter eviction. The index's eviction removed a transmission's subpaths but never decrementedspTotalPaths, sototalPathsgrew and everypctshrank over a long-running server. The old 215 test documented this. It is now the count of held transmissions with 2+ hops.FirstChunkReady, when HTTP binds) and the end ofLoadChunked, the three endpoints answer from the chunks merged so far, where master answered 503 until the whole hot window was merged and indexed. During the later background fill both trees answer from what is loaded. A partial answer can sit insubpathCache(60 s TTL) untilStartupLoadDone, which clears it (invalidateCachesFor{eviction}insignalStartupLoadDone); the same applies to the other analytics caches.avgSnr/avgRssican differ, by an ulp at most.maxMemoryMBnow holds more packets.Benchmarks (proof, AGENTS.md rule 0)
Setup:
go test -bench, Go 1.27.1, linux/amd64, i7-1260P VM with 4 vCPU, 3 runs each. The data issubpath413Data(30000, 2000, 413), the same in both columns. Master was measured back-to-back with the branch.0350322e/subpaths2–8, limit 100, uncached/subpaths-bulk, Route Patterns 4 groups, uncachedPacketStore.mu(RLock) is held/subpath-detail, hot pair (most frequent 2-hop)/subpath-detail, cold 5-hopReproduce with
cd cmd/server && go test -run '^$' -bench '_413' -benchtime 2s .. TheBenchmarkLegacySubpathIndex_413rows time the removed index through its frozen copy. They match the master measurements: 6.1 µs, 13.0 µs and 51.0 MB.Perf justification. An uncached list request is O(Σ min(path,8)·path) to enumerate, plus O(distinct keys) to resolve. The distinct-key work is the same work master did over
spIndex; the enumeration replaces it with a heap-bounded selection instead of a full sort of ~400K entries. Keys are built in a reused buffer, and a string is allocated only for a key not seen before. The transient maps are about the size of the old index (~400K keys) and are freed after the request, which is the +90 MB/op above. Results are cached per key for 60 s in the boundedsubpathCache.The detail matches the subpath against every packet's path, O(N·path), outside the lock (round 2, review F2). Under the first RLock it only copies each transmission's pointer and hop list into a slice allocated beforehand (~8 ns per packet); a second RLock copies what the detail reads of the matched transmissions, O(matches). Worst writer wait during one detail request on 30K tx (
BenchmarkSubpathDetailWriterWait_413, GC off, 3 runs): hot pair 2.5–3.3 ms → 1.06–1.25 ms, cold 5-hop 1.8–2.2 ms → 0.48–0.62 ms.Concurrent cache misses for one key share one count (round 2, review F1):
GetAnalyticsSubpaths,GetAnalyticsSubpathsWithWindowand the unfiltered bulk run their miss through onesingleflight.Group(subpathSF, keyed on the cache key, bulk on its groups' keys), which checkssubpathCacheagain inside the flight. Peak heap during N concurrent cold/subpathsrequests for one key on 30K tx (BenchmarkSubpathsColdConcurrent_413, 3 runs): N=1 141–146 → 136–147 MB, N=4 441–482 → 140–143 MB, N=8 907–914 → 137–143 MB.Tests
New:
cmd/server/subpath_on_demand_413_test.go,subpath_fixture_413_test.go,subpath_on_demand_413_bench_test.go.TestSubpathsOnDemandMatchLegacyIndex_413/subpaths(9 length/limit groups incl. ties at the cut, maxLen 63, minLen 9), bulk, window, region and detail (hot, cold, upper-case, 9-hop, 1-hop, unknown) vs the frozen index, before and after ingest, 100 path changes and eviction of 300; 2 seedsTestSubpathTotalPathsFollowsEviction_413EvictStale;totalPathsandpcttotalPaths = 2, want 1)TestSubpathCountsKeepNoPermanentIndex_413spIndex/spTxIndex/spTotalPaths; linear memory estimateTestSubpathCountingReleasesStoreLock_413TestSubpathRankTopNMatchesFullSort_413TestSubpathOnDemandConcurrentWriters_413-race: concurrent path, SNR, observer, append and evict writersExisting tests updated where they reached into the removed fields:
coverage_testcounts through the API.?window=. The unfiltered list never reached past 8 hops; the old test injected a key into the index directly./api/nodes/{pk}/pathswhile Route Patterns answer 200.Mutants: 18 run, 18 killed.
rawHopsnot the smallest raw keyTestSubpathsHonorsTimeWindow_StoreLeveltotalPaths-race)Validation
cd cmd/server && go test ./...: pass.cd cmd/ingestor && go test ./...: pass.-raceon the affected tests (_413, perf: move post-load index builds to background goroutines (startup Q1) Kpa-clawbot/CoreScope#1008, Deterministic order in GetSubpathDetail (same tie-order bug class as #256) #273, flaky: #226 Hash Stats adopters sort E2E — server sorts multiByteNodes on packets only, ties come out in random order #256, hash migrate, eviction, chunked load, tracked bytes): pass.sh test-all.sh: 259/259 files pass.node test-frontend-helpers.js: 719 passed.e2e-fixture.db, prepared as in CI (freshen, seed SQL,corescope-migrate, the four seed files):test-e2e-playwright.js: 154/157 passed, 3 skipped, including "Analytics Route Patterns tab renders content".test-a11y-1705-subpath-hop-prefix-e2e.js: pass.test-issue-1705-subpath-contrast-e2e.js: pass.window=7d),/subpathswith 4 length/limit combinations (including 2–63 and 9–63) pluswindow=24h, and 45 subpath-detail queries (the top 40 raw keys plus mixed-case and unknown ones).cmd/serverstays read-only: no new writes.map[string]interface{}in non-test Go code: 732 → 731.scripts/check-xss-sinks.sh --diff origin/masteris clean.deploy.yml, 1 inrelease-fast-path.yml).Coordination
#411 (PR #527) is merged and
origin/masteris merged into this branch (004a5644). The conflicts were comment-only. The combination turnedTestMemoryEviction_CrossesHighAndReachesLow_113red: itsmaxMemoryMBsat at a MiB boundary, so one pass had to free 25.03% against the 25% cap. The test now sizes its store, so that share stays between 15% and 20.4% (0aa39c41).🤖 Generated with Claude Code