Repository navigation
perf(store): compact concurrency-safe observation timestamp cache - #555
Conversation
…rser (#490) Differential tests compare ParsedTime's first and cached calls with the sync.Once parser it replaces (RFC3339/Nano, SQL, offsets, empty and invalid input, epoch, negative and zero times, the exact UnixNano range ends and the three instants whose encoding is a sentinel), assert the cache state each input leaves, and race first reads. Layout and accounting guards expect a 168-byte StoreObs charged at its 176-byte heap size class. A benchmark and a gated heap-after-load measurement use only the public behaviour, so they also run on master. Red: the cache field and its sentinels do not exist yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
) StoreObs kept its parsed Timestamp in a sync.Once, a time.Time and a bool: 40 bytes that made the struct 200 bytes, in the 208-byte heap size class. One atomic.Uint64 now holds either a sentinel (not parsed yet, invalid, reparse) or the time's UnixNano with its sign bit flipped, so the epoch and negative times cache like any other; only the first three instants UnixNano can hold (1677-09-21) share an encoding with a sentinel, and those reparse. A time with a non-UTC location, or one that UnixNano cannot give back exactly, is reparsed on each call, keeping its location and value. wireRoute keeps its padding byte; StoreObs is 168 bytes, size class 176, and storeObsBaseBytes now charges that class. A fuzz target runs the differential property on the seed corpus. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent1 PR#555 #490 — head 8b208f3Status: Implemented and locally green. CI is red on one job, Go Server Tests (3/3), from a pre-existing start-up/SIGTERM race in Evidence: [T] test or command run, with its result · [A] analysis/measurement by the author · [K] source code reference Requirements
Measurements
[A] Offset times reparse on every call. The store doesn't produce them on the v3 schema, because every load path selects Local validation
CI per job (run 40088547043)
Failure analysis:
Remaining items
|
|
Coordinator note: #551 (#489) is merged. git merges this PR cleanly, but the combined tree fails |
Review — CS-Minimax PR#555 — head 8b208f3Dom: REQUEST CHANGES The change itself is correct, and I could not break the Evidence: [T] test or command I ran, with its result · [A] my analysis or measurement · [K] source reference Findings
Answers to the review questions
Tests on this:
Concurrency.
#551 conflicts and merge order.
CI failure
Hygiene
Acceptance criteria (#490)
Tests and mutants (mine)Tests on the merged trees.
Mutants.
What I did not verify
|
#551 (#489) pinned StoreObs at 200 bytes with sharedStrs in padding; this PR shrinks the struct to 168 bytes. Merged together the layout is 168 bytes with wireRoute and sharedStrs still sharing the tail padding, so the guard and the store comment move to 168. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review feedback addressed by the coordinator: master (with #551) merged in as |
Relates to #490
Summary
StoreObsis the store's most numerous struct. Its lazy timestamp cache (Kpa-clawbot#1481) was async.Once, atime.Timeand abool: 40 bytes, which made the struct 200 bytes and put every observation in the 208-byte heap size class. The cache is now a singleatomic.Uint64.StoreObsis 168 bytes, in the 176-byte size class: 32 bytes less heap per observation.ParsedTime's contract is unchanged, andwireRoute(#76) keeps its padding byte.On a 1M-observation store the retained heap after load drops from 864.6 to 841.0 MiB (−23.6 MiB, −2.7 %). First parses cost the same. A cached UTC read goes from about 1.5 ns to 3.1 ns, because it now builds the
time.Timefrom an integer.Plan (two commits)
test(store): …(2dea959c): the differential, cache-state, concurrency, layout and accounting tests, plus a benchmark and a gated heap measurement. The benchmark and the measurement use only public behaviour, so they also produce the "before" numbers on master. The commit does not build, because the cache field and its sentinels don't exist yet. On master's code the layout and accounting guards fail with real assertions (see Tests).perf(store): …(8b208f36): the one-word cache,parseObsTimestamp, andstoreObsBaseBytesset to the actual size class. It also adds a fuzz target that checks the same differential property.There are no schema, config, API, workflow or deployment changes.
cmd/serverstays read-only.Change
cmd/server/store.go:ParsedTimereturns0(tsCacheNotParsed)1(tsCacheInvalid)time.Time{}, false2(tsCacheReparse)Timestampagain on each calluint64(UnixNano) ^ 1<<63time.Unix(0, n).UTC(), trueUnixNano. So the epoch (0), negative times and the zero point of every other scheme are ordinary cached values. Only the three instants whose encoding is a sentinel collide:1677-09-21T00:12:43.145224192Zplus 0, 1 or 2 ns, the first three instantsUnixNanocan hold. Those three are stored astsCacheReparse, so they are always correct. They are never misread.time.UTC(pointer comparison, soLocaland fixed+hh:mmzones are excluded), andtime.Unix(0, t.UnixNano())must equal it. The second check is exact at both range ends: it catches the silent wrap ofUnixNanooutside 1677–2262, with no conservative year cut-off. All other parsed times are reparsed, which keeps their location, offset and precision.parseObsTimestampis the old parse chain unchanged: empty →false, thenRFC3339Nano→RFC3339→"2006-01-02 15:04:05".Storethe same word. Reads are a single atomicLoad.StoreObs.Timestampis never assigned after construction (checked: the only.Timestamp =assignments incmd/serverare on other types), so a reparse reads the same input each time.storeObsBaseBytes192 → 176. The old constant was already 16 B under the 208-byte class it charged for. A test now pins the constant to the runtime's own size class forunsafe.Sizeof(StoreObs{}).Layout and accounting (64-bit)
9080b6aaunsafe.Sizeof(StoreObs{})storeObsBaseBytesestimateStoreObsBytes(TestEstimateStoreObsBytesobservation)wireRouteoffsettsParsedOK(192)tsCache(160)amd64 and arm64 give the same layout. I checked this with a compile-time
[168-Sizeof]byteassertion built for bothGOARCHes; it is not committed. The layout tests skip on 32-bit, as before.Coordination with #489 (PR #551, observer string pool, still open): that branch adds
sharedStrs uint8to the same padding block. With both changesStoreObsis still 168 B / 176 B, because the bytes aftertsCacheholdwireRouteandsharedStrs. The two branches will conflict textually in the struct and inheard_by_test.go's layout guard, and instoreObsBaseBytesif #551 touches it. If #551 lands first, I'll merge master into this branch (no rebase).Measurements
Environment: linux/amd64, 4 vCPU, go1.27.1. Before =
origin/master9080b6aawith onlyobs_time_cache_490_bench_test.gocopied in. After = this branch.First-parse and cache-hit cost
go test -cpu 1 -bench StoreObsParsedTime_490, before and after interleaved, 3 rounds × 4 runs, benchstat with n=12:…Z, first callYYYY-MM-DD hh:mm:ss, first call+01:00offset, first calltime.Unix(…).UTC()from the wordAllocations are unchanged in every case: SQL first parse 240 B / 6 allocs, invalid first parse 336 B / 9 allocs, all others 0.
Heap after load
CORESCOPE_MEASURE_490=<n> go test -run TestStoreObsHeapAfterLoad_490 -v. The test measures heap growth acrossNewPacketStore+Loadwith every observation's time parsed, after closing the DB and running GC twice:Loadproduces them)trackedBytestest-fixtures/e2e-fixture.db(499 observations)The measured saving is about 25 B per observation, against the 32 B size-class difference. The rest is within the run-to-run spread of about ±3 MiB on this machine. The fixture is too small to show the change: 499 × 32 B is 16 KB.
TestTrackedBytesTracksTheHeap_113's 0.85–1.15 band still passes.Trade-offs
/api/observers/{id}/analytics' 60k-observation loop that is roughly 0.1 ms per request, in exchange for 32 B per observation.Load, the chunked load andIngestNewFromDBselectstrftime('%Y-%m-%dT%H:%M:%fZ', o.timestamp, 'unixepoch'), which is always UTCZ. Only a legacy-schema text timestamp with an explicit offset would take this path, and it stays correct.-05:30), each reparse returns a new*time.Location, so==between two calls can differ whileEqual,Zone()andLocation().String()match. UTC andLocalvalues stay==-identical, and the tests assert this. Whole-hour offsets reuse Go's cached fixed zones. No caller comparesParsedTimeresults with==.Tests
New in
cmd/server/obs_time_cache_490_test.go, plus the updated guards inheard_by_test.goandeviction_test.go:TestStoreObsParsedTimeMatchesLegacyParser_490: 31 inputs, three calls each, against the replacedsync.Oncebody copied verbatim as the oracle. It checksok, instant,Location().String(),Zone()name and offset, RFC3339Nano rendering, and==for UTC/Local. Inputs: RFC3339 and Nano at 0/1/3/9 fractional digits, SQL, SQL epoch, epoch, ±1 ns around the epoch, 1900,+01:00,-05:30, nanosecond offset,+00:00(yieldsLocal), the zero time, year 0, 9999, the exactUnixNanomax and max+1 ns, min−1 ns, the three sentinel-colliding instants and the first non-colliding one, empty, space, garbage, date only, month 13, leap second, and no zone.TestStoreObsParsedTimeCacheStates_490: the word each input leaves after the first call (sentinel or the exact encoding), and that a cached call leaves it unchanged.TestStoreObsParsedTimeCachedCallDoesNotReparse_490: replacesTimestampafter the first call and checks that a cached answer (including "invalid") doesn't change.TestStoreObsParsedTimeConcurrentFirstReads_490: 8 readers released together, 20 rounds over all inputs, first and repeated calls. Run under-race.TestStoreObsLayoutTimeCacheIsOneWord_490,TestStoreObsLayoutFitsWireRouteInPadding_76(now 168 B,wireRouteattsCache+8), andTestStoreObsBaseBytesIsTheHeapSizeClass_490(constant equals the runtime size class, and 4096 allocations land in that class).FuzzStoreObsParsedTime_490: the differential property, seeded with the table. 60 s of-fuzzlocally (563k execs) found no divergence.Red before: on master's code the layout guard reports
unsafe.Sizeof(StoreObs{}) = 200, want 168, and the accounting guard reportsstoreObsBaseBytes = 192, want 208: the heap size class of a 200-byte StoreObs. The differential and race tests use the old parser as their oracle, so they pass on master by construction. The mutants below show what they catch.Mutants (each applied to the green tree, run, reverted)
uint64(n))tsCacheInvalidreparses instead of returningfalseCachedCallDoesNotReparsetsCacheReparse…145224193Zcomes backok=false0001-01-01comes back as 1754+01:00loses its locationUnixNanomax not cached.UTC()LocalvsUTCok=falsestoreObsBaseBytesback to 192TestEstimateStoreObsBytes, size-class guardStoreObswireRoutemoved out oftsCache's padding-raceDATA RACEtsCacheReparsedecoded as a timeRFC3339tried beforeRFC3339Nanotime.Parsesends both layouts through the sameparseRFC3339fast path, and the fallback accepts fractional seconds for either. It also survived 60 s of fuzzing.LoaddropswireRouteheard_by_test.go(route semantics)Validation
cd cmd/server && go test ./...: ok.cd cmd/ingestor && go test ./...: ok.go test -race -run 'ParsedTime|490|Retransmission|ObserverAnalytics|HeardBy|Route|Evidence': ok. The490|ParsedTimetests also pass withTZ=Europe/CopenhagenandTZ=Etc/GMT-1, where+01:00resolves toLocal.sh test-all.sh: 265 files passed.node test-frontend-helpers.js: 719 passed.e2e-fixture.db, prepared as CI does (freshen, inline seeds,corescope-migrate, seeds 2073/199/245/122), then stopped by pid:test-e2e-playwright.js154/157 (3 skips hard-coded in the suite),test-issue-76-heard-by-e2e.js8/8,test-issue-122-compare-reference-many-e2e.js11/11,test-issue-199-inactive-observer-e2e.js3/3,test-issue-1486-collapse-reopens-detail-e2e.js4/4,test-issue-147-packets-url-obs-e2e.js12/12,test-issue-1639-observers-sort-e2e.js,test-issue-497-rx-airtime-e2e.jsandtest-observer-iata-1188-e2e.jsall passed.scripts/check-xss-sinks.sh --diff origin/master: no frontend changes. No newmap[string]interface{}. No.githubchanges; the fork guards are unchanged (9 indeploy.yml, 1 inrelease-fast-path.yml).🤖 Generated with Claude Code