Repository navigation
perf(nodes): per-IATA region membership, union per request, warm at startup - #619
Merged
Merged
Conversation
…tartup (#618) A region-filtered /api/nodes request used to run a full history scan per region set (observers x observations x ADVERT transmissions), so every new set or quick pick cost about a minute on a large database after a restart. The heard rule now shares the #488 M2 machinery with placement: one global region index per rule (region_index.go), kept by one shared scan grouped by observer IATA, with the aggregate's watermark, delta scans, periodic full rebuild, singleflight and immutable snapshot. A region set is the union of its codes' member lists, cut into the existing bounded per-set cache. - StartRegionIndexWarmer builds the index after startup and refreshes it every nodeRegionFreshTTL, so the full scans and rebuilds run outside any request (and outside PacketStore.mu: it reads SQLite only). - A request with a usable (stale) snapshot is cut from it at once and only kicks a refresh; it never queues behind a running rebuild. - Before the first snapshot, a waiting request races its own per-set build (master's query, serialised) against the index build; identical sets share one build. - Entries carry the snapshot's ages, so the fresh/stale/max-stale contract is that of the data. - The full heard scan drives from the ADVERT transmissions and dedupes in Go instead of a SQL DISTINCT. Tests: union equals the per-set build for every subset on a generated DB, the e2e fixture and (env-gated) large generated DBs; one full scan serves every set; warmup, keep-alive, cold race both ways, no queueing behind a rebuild, warmer stop, bounded memory, v2 schema, source guards, and the server process warming at startup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
Rapport — CS-pve-agent1 PR#619 #618 — head 51e62b8Status: Draft, implemented as written; all local suites and mutants green; CI: all jobs green on the first attempt. One known limit (cold window on many equal small regions) is documented below. Evidence tags: [T] run for this report (test, mutant, benchmark or command output), [A] argued from the code, not executed, [K] carried over from the issue, CI or an earlier run, not re-verified. "Red before": a test either fails on Requirements
Mutation runner: 23 mutants, 23 killed, each on a failing assertion (none on a compile error) [T]. Benchmarks (local, 4 vCPU, medians of 5) [T]
Full-scan variants on the #176 DB: planner + Local verification [T]
CI per jobRun 40286467215 on head 51e62b8, first attempt, no reruns (the known flaky #271 did not fire) [T]:
Go Server shard 3 at 20m: another PR branch's recent run took 19m and master's 12m [T]. The new #618 tests take about 1.4 s together locally [T], so the difference is runner variance, not this change [A]. Rests
|
Collaborator
Author
Review — CS-MacBook PR#619 — head 51e62b8Dom: APPROVE with nits. The owner still has to accept the cold-window deviation from the acceptance criteria explicitly (F1). Evidence tags: [T] run for this review, [A] argued from the code and not executed, [K] carried over from the author's report, the issue or CI and not re-run. Trees: head Findings
F1 in detailThese are my own measurements on the merged tree with the author's generators, medians of 5, local [T]. A reviewer-only test (not committed) starts the real warmer, then makes one cold request for the largest single code at an offset into the warmup. #176 generator: 1.5M obs, 13 regions. Largest code alone takes 234 ms; the index build alone takes 405 ms.
#488 generator: the index (57 ms) is cheaper than the per-set build (141 ms), so the index always wins: 0.13–0.55× of today. So the race helps the requester only very early in the warmup. After about the first fifth of the warmup it is slower than waiting. In every case it delays the one build that serves every other region set, by up to 2× (809 vs 405 ms). Production shape [A]. The per-set plan was recorded (in the comment this PR removes) as driving off This is a trade-off, not a correctness bug. The PR is a large net win in any case:
Answers to the verification points
Tests run (merged tree)
The E2E server ran on
CI. Run 40286467215, attempt 1: every job passed. Publish Badges, Release Artifacts and Deploy Staging were skipped by the fork guards. The known flakes #256 and #267 did not fire. [T] Hygiene. [T]
Mutants (run on the merged tree, filter
|
| Mutant | Change | Result |
|---|---|---|
| M2a | members() unions only the first code |
killed: 9 tests, incl. UnionEqualsPerSetBuild, the fixture test and the #176 cache test [T] |
| M2b | heard scan without UPPER() |
killed: UnionEqualsPerSetBuild, WritePathShapesMatchJSONExtract_PR38 [T] |
| M4a | delta not merged (agg.merge(delta) dropped) |
killed: 9 tests, incl. the #176 delta tests, WarmerKeepsIndexCurrent, BoundedMemory and the #488 tests [T] |
| M6a | a new set waits for the index refresh (currentRegionIndex(…, true)) |
killed: RequestsDoNotQueueBehindRebuild [T] |
| O1 (own) | the cold race's index build runs under a ctx cancelled when the request returns | killed: 10 tests [T] |
| O3 (own) | the heard scan no longer raises agg.watermark, so every delta becomes a full rescan |
killed: 9 tests [T] |
| O2 (own) | idle refresh does not re-stamp refreshed |
survived → F4 [T] |
| O4 (own) | queued per-set build does not give up after the index won | survived → F3 [T] |
| O5 (own) | index failure in the cold path returns at once, no per-set fallback | survived → F2 [T] |
All kills were on failing assertions, not compile errors.
Red before green. The new tests use seams that do not exist on master (regionHeard, startRegionIndexWarmer, waitForHeardIndex), so they do not compile there. I relied on the author's mutant table for "red before" on the acceptance criteria not covered above: M1, M3a–c, M5, M7a/b, M8a–c, M9–M13 [K]. M2a/M2b/M4a/M6a were re-run here [T].
Not verified
- Production or staging behaviour, plans and timings. Everything about the 67M-observation, 2-vCPU host is [A], including the assumed ADVERT share of observations and the WAL effect. Nothing was measured there, by instruction.
- The author's mutants other than M2a/M2b/M4a/M6a [K].
- Red-before on master for the new tests (compile-gated) [K].
- Browser validation beyond the Playwright suites listed above; there is no visual change.
- The full CI E2E shard set locally. I ran only the PR's own and the region-related E2Es plus
test-e2e-playwright.js.
…, queue and idle re-stamp (#618) Review round 2 of PR #619 (F1-F4). Red on 51e62b8's race: - ColdRequestsDuringWarmup: cold requests during the warmer's held build are not served before it lands and run no per-set build (F1). - ColdRequestFallsBackWhenIndexFails: a failing index build falls back to the per-set build, not a 500; once the index builds, new sets are cut from it (F2, kills O5). - QueuedFallbackTakesRecoveredIndex: a fallback queued behind nodeRegionColdMu is cut from an index published meanwhile (F3). - IdleRefreshReStampsIndex: an idle refresh re-stamps the snapshot without scanning (F4, kills O2). ColdRequestTakesIndexWhenFirst becomes ColdRequestsShareOneIndexBuild, ColdPerSetBuildsAreSerialised now runs with a failing index, and the perf test times cold requests at 0/25/50/75 % of the warmup. Adds the nodeRegionPerSetHook test seam. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…618) Review round 2 of PR #619. The owner chose F1 option 1: drop the race between the index build and a per-set build. Before the first snapshot a request waits for the index build the warmer already has in flight (singleflight), so it waits at most the rest of the warmup and adds no second full scan that slows the one build every set is served from. - F2: only when that index build fails does the request fall back to its own per-set build, so a failing index is not a 500. - F3: fallbacks stay serialised under nodeRegionColdMu (now taken in coldNodeRegion), and one still queued when a retry published the index is cut from it instead of scanning. The old ctx.Err() check and the WithoutCancel timeout in buildNodeRegionCold were only reachable when the index won the race, so they are removed. - F4 needed no code change; the idle re-stamp is now tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
Rapport — CS-pve-agent1 PR#619 runde2 — head 6d63333Review feedback addressed (commit
Evidence tags: [T] run for this report, [A] argued from the code, not executed, [K] carried over from the review or an earlier run. Acceptance (restated in the PR description)
The perf test measures this directly. The request's latency equals the rest of the warmup within 1–3 ms at every offset, on both DBs (table below) [T]. After warmup, the worst cut of a new set is 0.52 ms / 1.28 ms and a cached hit takes 262 ns / 850 ns [T]. Cold / warm, both generator DBs (local, 4 vCPU, medians of 5) [T]Same machine, same #176 generator (1.5M obs, 13 regions; SJC per-set alone: 346 ms wait run / 334 ms race run)
#488 generator (1.07M obs, 4 regions; BBB per-set alone: 173 ms / 180 ms)
The trade-off the owner accepted shows on the #176 shape. In the first quarter of the warmup the requester waits longer than with the race (745 vs 478 ms at 0 %). In exchange the warmup is no longer slowed by a competing scan, and every other set waits for it: 0.74 s instead of 1.40 s. From about half the warmup on, waiting is also faster for the requester itself. On the #488 shape waiting is faster at every offset. Other numbers (wait build): index cold build of all codes 107 ms / 730 ms, against 1.28 s / 4.91 s for master's per-set builds summed; index heap 1.3 MiB / 7.9 MiB. With no warmer, a cold request starts the build itself: 110 ms / 730 ms. Large-DB union property: #488 15/15 sets and #176 92/92 sets equal the per-set build, 1 full scan each. [T] Tests and mutants [T]
Mutation run (filter
Local verification [T]
CI per jobRun 40304631701 on head 6d63333, first attempt, no reruns; the known flaky #271 and #301 did not fire [T].
Not verified
|
Collaborator
Author
Review — CS-MacBook PR#619 — head 6d63333Dom: APPROVE with nits Round 2. Evidence tags: [T] run for this review, [A] argued from the code and not executed, [K] carried over from the author's report, CI or round 1 and not re-run. Trees: head Findings
Verification points
These numbers match the author's ratios: their absolute times are about 1.8× higher on a slower machine. They also match round 1's estimate of "just waiting for the index" (~405 / 304 / 203 / 101 ms). Tests and mutants
E2E. Local Go server on port 13900 against
Mutants. All run on a merged-tree copy with the filter
Red before green. On Hygiene. [T]
Not verified
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #618
Problem
/api/nodes?region=…built its node set with a full history scan per region set (observers ⋈ observations ⋈ ADVERT transmissions), cached under the joined codes. After a restart every new set — a single code, a combination, a quick-pick group — paid that scan again, about a minute each on a large database, while the Nodes page showed nothing.Plan (implemented in this PR)
regionIndexin the newregion_index.goholds aregionEvidenceAggwith anobservations.idwatermark, delta scans past it, a full rebuild everynodeRegionRebuildInterval, a singleflight and an immutable snapshot (region → sorted members). The two rules differ only in their scan and in how a node's per-region evidence becomes member regions (regionIndexSpec).currentRegionPlacement/refreshRegionPlacement/placedNodeRegionand the heardbuildNodeRegion/extendNodeRegionare folded into that one driver.StartRegionIndexWarmer(started inmain, stopped before the DB closes) builds the index right after startup and refreshes it everynodeRegionFreshTTL: a delta scan normally, the full rebuild when due. It reads SQLite only, never the PacketStore orPacketStore.mu.idx_transmissions_payload_type→idx_observations_transmission_id) and dedupes in Go instead of a SQLDISTINCT. Without a region filter the planner walked every observation by rowid and looked up its transmission (0.3–0.7× master's all-codes time, below).Acceptance (restated, owner decision in review round 1)
The issue's "the cold path never blocks longer than today" is replaced by:
Waiting for the warmup can be longer than master's per-set build for a request that arrives right at boot on a DB with many equal small regions (measured below: 745 vs 346 ms at 0 % on the #176 generator). In exchange the warmup is not slowed by a competing scan, so every other set is served earlier (warmup 0.74 s instead of 1.40 s there).
Server stays read-only; no new
map[string]interface{}; no frontend, config or customizer change (the intervals stay the existing constants).Equivalence
TestNodeRegionIndex618_UnionEqualsPerSetBuildcompares every non-empty subset ofAAA…EEE,ZZZ(63 sets plus normaliser spellings) against master's per-set query, kept verbatim as the oracle, on a generated DB with awkward observers: lower-case/padded IATA, NULL/blank IATA, an ADVERT withoutfrom_pubkey, a non-ADVERT. It runs cold, after a delta and after a rebuild that has to move a node between regions. The same property runs on the e2e fixture (31 sets) and, env-gated, on both large generated DBs:Performance (local, 4 vCPU, medians of 5, re-measured in round 2)
TestPerf_RegionPlacementM2_488on the #488 DB: heard delta refresh over the newest 5,000 observations 3.5 ms, one delta scan of 5,000 ADVERT observations for all regions 7.1 ms.Full heard scan variants measured during the work (#176 DB): planner order +
DISTINCT1.21 s, transmission-first +DISTINCT1.24 s, transmission-first + Go dedupe 0.87 s (chosen).Commands:
Complexity / scale. The full scan is O(ADVERT observations) once per
nodeRegionRebuildIntervalin the warmer, instead of once per region set per interval. A delta is a rowid range. A snapshot is O(pairs). A cut is O(members of the set's codes) plus a sort and JSON encode, at most once pernodeRegionFreshTTLper set. Memory is bounded by distinct (node, region) pairs (TestNodeRegionIndex618_BoundedMemory); the per-set cache stays at 32 entries.Round 2 replaced an earlier race between the index build and the request's own per-set build. Same machine, same test, the race build: #176 generator 478 / 481 / 481 / 403 ms at 0 / 25 / 50 / 75 %, with the warmup slowed to 0.95–1.40 s; #488 generator 158 / 120 / 81 / 48 ms, warmup 128–158 ms.
Known limits
nodeRegionRebuildIntervaleven with no region traffic (107–730 ms here). Master scanned only when a region set was requested, but scanned once per set.Tests
New:
cmd/server/nodes_region_index_618_test.go(property and fixture equivalence, delta = full, warmup, keep-alive, cold requests wait for the warmup's build, cold requests share one index build, per-set fallback when the index build fails, queued fallbacks take an index published meanwhile, fallbacks serialised, idle refresh re-stamps without scanning, no queueing behind a rebuild, warmer stop, bounded memory, v2 schema, source guards, the server process warming at startup, env-gated large-DB property and perf). Adapted: the #176 and #488 M2 tests age the index together with the entry and use the shared seams (regionPlaced.*).🤖 Generated with Claude Code