Repository navigation
perf(store): avoid eager per-transmission observation dedup maps - #527
Conversation
Red: the helpers in obs_dedup.go are a stub over today's eager maps. The new tests fill a store through hot Load, LoadChunked, the background fill, IngestNewFromDB and IngestNewObservations from one skewed synthetic set (log-normal observations per transmission, plus busy transmissions with exact repeats past the threshold), and check against the rows: retained observer/path pairs, unique observer counts, relay indexes (byPathHop, byNode, pathHopResolved, fallbackByNode), totalObs and trackedBytes, then retention and maxMemoryMB eviction, a path-hop index rebuild, the in-memory hash merge and a same-row route_mask upsert. They fail only where a transmission below obsIndexThreshold carries maps and where the estimate charges them. obs_dedup_411_data_test.go also holds the measurement (CORESCOPE_OBS411_MEASURE) and BenchmarkLoad_Skewed30K_411. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every StoreTx carried two maps (obsKeys keyed by a concatenated "observerID|pathJSON" string, observerSet) from creation. On a skewed 30K-transmission set (median 4 distinct observer/path pairs, p99 21, p99.9 36) that is about 1 KB per transmission for lists that are usually under ten long. - lookupObservation scans tx.Observations below obsIndexThreshold (32), comparing the strings the observations already hold, and answers the duplicate and the known-observer question in one pass. - From 32 observations on, addObservation builds obsKeys (keyed by an obsDedupKey struct that shares those strings) and observerSet, so busy transmissions keep O(1) checks. - Hot load, LoadChunked, the background fill, IngestNewFromDB, IngestNewObservations and the hash merge all go through lookupObservation/addObservation; the counter updates that were copied at each site live there. - trackedBytes no longer charges per-transmission maps or a key copy per observation. The maps of a busy transmission are charged by obsIndexBytes, returned as a delta by addObservation and credited through txChargedBytes, so eviction stays exact. Heap after a hot Load of the skewed set: 145.4 -> 115.2 MiB (-20.8%). Existing tests that pinned the removed estimate terms or read the maps directly are updated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent1 PR#527 #411 — head e0f7eb7Status: Draft PR implements #411. All requirements are covered by red→green tests with mutants. CI is green on every job with no reruns. Not merged and not ready, by design. Legend: [T] = test evidence (run locally or in CI), [A] = analysis/measurement, [K] = known limitation or residual. Commits
Requirements
On the red commit the equivalence halves (keys, counts, relay indexes, Mutants (each applied alone on
|
| # | Mutant | Result |
|---|---|---|
| M1 | scan ignores the path | killed (9 tests) |
| M2 | an unknown observer is counted | killed (5) |
| M3 | maps built from the first observation | killed (8) |
| M4 | maps not updated past the threshold | killed (6) |
| M5 | txChargedBytes omits obsIndexBytes |
killed (3) |
| M6 | background fill drops the map charge | killed (4) |
| M7 | IngestNewObservations admits duplicates |
killed (4) |
| M8 | hash merge keeps shared pairs twice | killed (2) |
| M9 | observerSet built with the empty observer |
killed (3) |
| M10 | map lookup ignores observerSet |
killed (7) |
| M11 | ingest skips relay-hop indexing for later observations | killed (2) |
Benchmarks (before = red commit, which runs master's store code; benchstat, n=6) [T]
| Benchmark | sec/op | B/op | allocs/op |
|---|---|---|---|
Load_Skewed30K_411 |
−8.6 % (p=0.015) | −8.6 % | −3.3 % |
Load_30K_Unlimited |
−9.4 % (p=0.004) | −10.7 % | −4.4 % |
IngestNewFromDB_158 / IngestNewObservations_158 |
~ | −2.2 % / −0.3 % | −1.8 % / −1.0 % |
| heap after hot load, 30K skewed | 145.4 → 115.2 MiB (−20.8 %) |
Local runs [T]
cmd/server go test ./...: okcmd/ingestor go test ./...: oksh test-all.sh: 259/259node test-frontend-helpers.js: 719 passed- Local Go server with
e2e-fixture.dbprepared as in CI (freshen, the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed, migrate, the four seed SQL files):test-e2e-playwright.js: 154/157 passed, 3 skipped- 16 E2Es that touch observation or observer counts / Heard By: all pass
- The server was stopped by pid afterwards.
CI per job (head e0f7eb77) [T]
| Job | Result |
|---|---|
| Go Server Tests (1/3, 2/3, 3/3) | pass |
| Go Ingestor Tests | pass |
| Go Libraries & CLI | pass |
| Go Build & Test | pass |
| JavaScript Unit Tests & Lint | pass |
| Script & Container Unit Checks | pass |
| Playwright E2E Tests (1/3, 2/3, 3/3) | pass |
| Verify E2E Shards & Coverage | pass |
| Verify Go Test Assignments | pass |
| Validate Docker Image | pass |
| Build & Publish Docker Image | pass |
| Publish Badges & Summary / Release Artifacts / Deploy Staging | skipping (PR) |
The known flaky test (#271) did not fail, so no rerun was needed.
Residuals
- [K] tracked/heap ratio.
trackedBytesnow over-charges slightly: tracked/heap is 1.06 on the skewed set (was 1.03) and 1.13 on the data ofTestTrackedBytesTracksTheHeap_113(was 1.06; the bound is 1.15). This is conservative, because eviction triggers earlier, never later. RecalibratingstoreTxBaseByteswas left out on purpose, because it would change budget enforcement for every transmission. - [K] The fixture DB has only 1 observation per tx, so the threshold rests on the synthetic set plus the upstream-reported real shape. A heap measurement on staging is left to the coordinator.
- [K]
obsIndexThresholdis a code constant, so there are no customizer implications. - [K] perf(store): evaluate on-demand subpath counts instead of permanent indexes #413 had not merged when this branch was cut from
0350322e, so no sync was needed. If it merges first, the overlap instore.gois limited to the six dedup sites and the estimate constants. - [K] A local Go build cache got corrupted once ("bad reloc size") during the benchmarking. A fresh
GOCACHEfixed it; CI builds clean.
Review — CS-pve-agent3 PR#527 — head e0f7eb7Dom: APPROVE med nits Legend: [T] = test or benchmark run by me, [A] = analysis or code reading, [K] = known limitation or not verified. Independent, read-only review. Everything ran on Findings
Answers to the review pointsQ1. Threshold vs a prod-like distribution. The public production
Q2. Worst case of the scan on hot paths.
Each call is O(min(k, 31)), and from 32 on it is O(1). The worst case is therefore O(31²) per tx while it is being built, and O(k_loser · 31) per hash merge. No path calls it in a loop over all observations of all tx. Q3.
Q4. -race, #495 lock guard, #529.
Standing requirements
Tests and mutants (merged tree on
|
| # | Mutant | Result |
|---|---|---|
| MX1 | loser maps cleared before the loser is credited (finishHashMerge) |
survived (F1); killed by the extra 36-per-row case |
| MX2 | observerSet not seeded from the existing observations when the maps are built |
killed (7 tests) |
| MX3 | moveObservations drops the addObservation charge delta |
killed (HashMergeDedupsAcrossTheThreshold) |
| MX4 | threshold off by one (> instead of >=) |
killed (HashMergeDedupsAcrossTheThreshold, ThresholdAndAnswers) |
Not verified
- [K] The real observation distribution beyond the mean: the public aggregates give only obsPerTx, so the p99 ≥ 60 tail is modelled.
- [K] Heap on staging or production: no access, by design.
- [K]
-raceon the fullcmd/serverpackage and oncmd/ingestor: only the store-related subset above was run. - [K] CI on the merged tree, and the merge with
e14cac27beyondgit merge-tree(clean, and it touches no store code). - [K] Browser screenshots: the PR has no UI changes.
- [K] The author's light-set numbers (−20.8 % heap) were not re-run; only the heavy distribution above was measured.
#411) Add the 'rows already past' case so a loser holds maps when it is merged, and correct the threshold rationale in obs_dedup.go (comment only). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent1 PR#527 runde-2 — head 71a21eeLegend: [T] = test run by me, [A] = analysis or code reading, [K] = known limitation or not verified. Review feedback addressed (commit
Branch: merged Tests
CI on head 71a21ee (run attempt 1, no reruns)All pass: Go Build & Test, Go Ingestor Tests, Go Libraries & CLI, Go Server Tests 1/3, 2/3 and 3/3, JavaScript Unit Tests & Lint, Script & Container Unit Checks, Playwright E2E 1/3, 2/3 and 3/3, Validate Docker Image, Build & Publish Docker Image, Verify E2E Shards & Coverage, Verify Go Test Assignments. Skipped by the workflow: Publish Badges & Summary, Release Artifacts, Deploy Staging. No flaky tests (#271, #301) were hit. [T] |
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>
Relates to #411
Problem
Every
StoreTxcarried two maps for observation dedup from the moment it was created:obsKeys, keyed by a concatenatedobserverID + "|" + pathJSONstring that was allocated per observation, andobserverSetforUniqueObserverCount. Most transmissions are heard a handful of times, so these maps and their keys cost about 1 KB per transmission to deduplicate lists that are usually under ten long.Plan (one change, two commits)
test(store): …: the equivalence tests plus a stubobs_dedup.gothat keeps today's eager maps behind the new helpers. They fail only on "a transmission below the threshold holds maps" and on the estimate terms.perf(store): …: the implementation.Current master was checked first. The Regions extras are gone (#523/#524). #495 (no SQL under
PacketStore.mu), #496 and #511 changed the load paths, but none of them touches the dedup sites. Kpa Kpa-clawbot#2181 was used as inspiration only: our fork additionally has the background fill, the in-memory hash merge (#215) and exacttrackedBytesaccounting (#113/#164/#202), and all three are handled here.Measurements before choosing the threshold
test-fixtures/e2e-fixture.db: 499 transmissions and 500 observations, i.e. 1 per transmission (one has 2). It is too small to choose anything from, so the synthetic set below is the basis.Synthetic 30K set (
obs411Data,obs_dedup_411_data_test.go): log-normal rows per transmission (median 4, σ 0.75), 48 64-hex observers, one of 3 paths per row, about 1 % unknown observers. This produces exact repeats and a second path from the same observer naturally.For comparison, Kpa Kpa-clawbot#2181 reported a mean of 6.25 and a max of 40 on a real 7-day window. That is the same shape, and the synthetic set has the heavier tail.
Per-check cost (
BenchmarkObsDedup, a check that misses, which is the ingest common case; median of 3). Adversarial means one observer on k equal-length paths that differ only at the end, the worst case for the scan.Threshold:
obsIndexThreshold = 32.Change
obs_dedup.go:lookupObservation(observerID, pathJSON) (dup, knownObserver)answers both questions in one pass: a scan below the threshold, and from the threshold on a map keyed byobsDedupKey{observerID, pathJSON}, which shares the observations' strings, so no key is allocated.addObservation(obs, knownObserver)keepsObservations,ObservationCount,UniqueObserverCountand the maps in step, builds the maps when the list reaches 32, and returns the change inobsIndexBytes.Load,scanAndMergeChunk(LoadChunked),loadChunkWhere(background fill),IngestNewFromDB,IngestNewObservationsandmoveObservations(hash merge).trackedBytes/maxMemoryMB:perTxMapsBytes(200 B/tx) and per-observation key terms (obsKeyEntryBytes+ observer + path) are removed, because nothing allocates them any more.obsIndexBytes, a function of the map sizes alone: 96 B + 88 B per key + 40 B per observer, measured on Go swiss maps with 32–600 entries. The charge is added as a delta at every insert and credited throughtxChargedBytes, following the same pattern asresolvedRelayBytes, so eviction stays exact.StoreTxstays 320 bytes. The two fields keep their place and are simply nil below the threshold.Not changed:
cmd/serverstays read-only, there are no newmap[string]interface{}, no frontend or config changes, and no workflow changes.Equivalence tests (
obs_dedup_411_test.go)One skewed set is used: 600 ordinary transmissions, half of them 3 days old, plus 3 busy ones with 600 distinct rows and 40 exact repeats past the threshold. It goes through five fill paths: hot Load, LoadChunked, background fill (LoadChunked plus loadChunk), ingest (
IngestNewFromDBfrom empty), and upsert (first row viaIngestNewFromDB, the rest viaIngestNewObservations). Every path is checked against the rows themselves and against hot Load.PathsKeepTheSameObservationsAndIndexes,ThresholdAndAnswers,TestObsDedupCorrectnessHashMergeDedupsAcrossTheThresholdSameRowRouteUpdateKeepsObservationsbyPathHop,byNode,pathHopResolved,fallbackByNode)PathsKeepTheSameObservationsAndIndexes,RebuildKeepsIndexesAndChargestrackedBytesexact + identical across pathsacct113Check(now includingobsIndexBytes),EstimateHasNoEagerMapTermsEvictionCreditsWhatEachPathChargedmaxMemoryMBevicts the same tx on every pathMemoryBudgetEvictsTheSameOnEveryPathbuildPathHopIndex) changes nothingRebuildKeepsIndexesAndChargesHashMergeDedupsAcrossTheThresholdThresholdAndAnswers(AllocsPerRun == 0)On the red commit, the equivalence halves (counts, keys, relay indexes,
trackedBytesacross paths) already pass. Master was equivalent across paths, and the red comes from the structural assertions and the estimate terms. The mutants show that the equivalence halves have teeth. All 11 mutants were killed; the details are in the report comment.Existing tests that pinned the removed estimate terms or read
observerSetdirectly are updated:eviction_test.go,tracked_bytes_accounting_113_test.goandhash_migrate_readonly_215_test.go.obs_dedup_test.gonow goes through the helpers and holdsBenchmarkObsDedup.Benchmarks (before = red commit, which runs master's store code;
-count=6 -benchtime=5x, benchstat)Load_Skewed30K_411Load_30K_UnlimitedIngestNewFromDB_158IngestNewObservations_158TrackedBytes_IngestNewTx_113TrackedBytes_IngestNewObs_113TrackedBytes_Evict_113Heap after load (
Load_Skewed30K_411heapMiBmetric andCORESCOPE_OBS411_MEASURE=30000, 3 runs each): 145.4 MiB → 115.2 MiB (−20.8 %), 5,082 → 4,026 B/tx.trackedByteswent from 149.0 to 122.5 MiB, so tracked/heap moved from 1.03 to 1.06. On the data ofTestTrackedBytesTracksTheHeap_113the ratio moved from 1.06 to 1.13. That test's bound is 0.85–1.15, and it passes. The heap dropped by more than the removed estimate terms, sotrackedBytesnow over-charges slightly, which means eviction triggers a little earlier, never later. I leftstoreTxBaseBytesunchanged on purpose, because recalibrating it would change budget enforcement for every transmission. That makes it a candidate follow-up (see below).Perf justification (AGENTS.md rule 0):
Validation (local)
cd cmd/server && go test ./...: okcd cmd/ingestor && go test ./...: okgo test -raceon the store/load/eviction/ingest/hash-migrate/route-mask tests: ok, no racessh test-all.sh: 259/259node test-frontend-helpers.js: 719 passede2e-fixture.dbprepared as in CI (freshen, the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed, migrate, the four seed SQL files):test-e2e-playwright.js: 154/157 passed, 3 skippedtest-issue-76-heard-by-e2e.js,test-issue-165-grouped-hop-warn-e2e.jsandtest-packet-detail-sender-hash-size-obs-e2e.jsscripts/check-xss-sinks.sh --diff origin/master: no frontend changesdeploy.yml, 1 inrelease-fast-path.ymlNo staging or production access was used; heap on staging is left to the coordinator.
Follow-up
trackedBytesbase terms. The ratio to the heap is now 1.06–1.13 (it was 1.03–1.06), conservative but closer to the 1.15 bound ofTestTrackedBytesTracksTheHeap_113.obsIndexThresholdis a code constant, not a config knob. It is a memory/CPU trade-off rather than a user-facing value, so there are no customizer implications.🤖 Generated with Claude Code