Count deliveries per stored second across a whole night (#1331/#1008) - #1486
Merged
Conversation
`ord` restarts at 0 on every delivery, so two rows on one second both carrying
ord 0 came from two different offloads writing the same wall second. Today's
5/MG log shows exactly that - `-1s[872#0, 893#0]` - but `densestSecondWindowSample`
only dumps the 5-8 seconds around the densest one. That is a sample, not a
measurement.
The fix turns on a question the sample cannot answer: is the over-count mostly
seconds touched by SEVERAL deliveries, or genuinely too many beats inside one?
If it is the former the fix belongs at ingest and this sizes it; if it is not, we
would be about to write the wrong fix.
rr deliveries secs[1/2/3/4+]=2/1/0/0 multiSec=33% multiRows=33% maxDeliv=2
secsNoStart=0 ordUnknown=0
Read-side only. It needs no new capture and no write-path change, so it lights up
on data already stored the moment someone opens the app.
Two things it deliberately refuses to flatter, both found on re-review:
multiRows divides by ATTRIBUTABLE rows (those carrying an ord), not by every row.
Dividing by the total would let a night that half-predates the ord column read
artificially benign - the exact conclusion this exists to prevent.
secsNoStart counts seconds carrying rows but no ord-0 row at all. That is
reachable: the primary key absorbs a cross-batch exact duplicate and the row it
drops can be the delivery's first on that second. Reporting it keeps `secs` from
quietly shrinking the denominator underneath multiSec.
Percentages are integer half-up on both platforms, so a tie cannot render
differently per platform (the #1473 lesson).
Verification: mirrored tests on both platforms asserting the rendered line
verbatim - the two-delivery shape from the field log, a clean night, and the
nil-ord case - and the three expected strings were compared across the two suites
and are identical. compileFullDebugKotlin clean, full Android unit suite green,
HrvAnalyzerSampleOrdTest 7, doc_comment_lint clean. StrandAnalytics needs macOS,
so its suite runs in CI.
Third re-review. The function took rrMs and never read a value from it - only its count, to bound the loop. A parameter a reader would assume matters, doing nothing. The right response was to use it, not drop it. Coverage is the sum of rrMs over the wall span, so BEAT-TIME is what inflates it; row count is a proxy. multiMs now reports the share of attributable beat-time carried by seconds that several deliveries wrote, and that is the number the fix is sized against. It can also disagree with multiRows in a way that matters: a high row share with a low beat-time share would mean the extra rows are short and barely move coverage, which is a different problem from the one this is chasing. Reporting only rows would have hidden that distinction. The test caught me guessing. I wrote 42% by eye; the real value is 36% - second 100 carries both ord-0 rows, 872+893 of 4911 ms attributable. Corrected, and worth recording that the assertion did its job rather than being fitted to the output. Verification: mirrored tests on both platforms, three expected strings extracted from the two suites and compared - identical. compileFullDebugKotlin clean, full Android unit suite green, HrvAnalyzerSampleOrdTest 7, doc_comment_lint clean.
Fourth re-review, parity pass. The two implementations rounded beat-time with DIFFERENT functions: Swift `.rounded()` is half-away-from-zero, Kotlin `kotlin.math.round` is half-toward-positive-infinity. They agree here only because these sums are positive. That is the same shape as #1473, where `%.1f` looked equivalent across platforms and was not - Foundation rendered 50 ms as 0.0 while Java rendered 0.1. An agreement that holds only for the inputs we happen to feed it is not parity, it is a coincidence waiting for a negative or a different call site. Both now use `x + 0.5` truncated - half-up, no stdlib rounding on either side, so the agreement is structural. Pinned by a mirrored test including 2.5 -> 3, which half-to-even would render as 2. Also verified this pass, mechanically rather than by eye: the two functions match on all seven structural decisions (ord-0 test, the >= 2 multi threshold, the 4+ histogram clamp, the unknown skip, the beat-time sum, msToInt, secsNoStart), and the three rendered strings extracted from both test suites are identical. Verification: compileFullDebugKotlin clean, full Android unit suite green, HrvAnalyzerSampleOrdTest 8, doc_comment_lint clean.
Optimisation pass. The function kept FOUR collections keyed by the same second - secsSeen, knownRowsPerSec, knownMsPerSec, deliveriesPerSec - so every row paid three or four hash lookups and the night allocated four maps of ~30k entries. That runs once per over-counted night, and analyzeRecent re-scores ~21 days every 15 minutes. At ~70k rows a night that is on the order of a million row-iterations and several million hash lookups per cycle, on a phone, for a diagnostic that feeds nothing. One map of a small tally now does the same work with one lookup per row and one allocation per second. Output is unchanged - the tests assert the rendered line verbatim and pass untouched, which is what makes this safe to do at all. The Swift tally is a CLASS, not a struct, deliberately: a struct is a value type, so accumulating into it means read-modify-write-back, two lookups per row, which throws away half the saving. Mutating through a reference keeps it at one and matches the Kotlin twin's shape exactly. An earlier revision of this commit used a struct and the doc claimed one lookup while the code did two - the claim was corrected by fixing the code, not the sentence. Verification: compileFullDebugKotlin clean, full Android unit suite green, HrvAnalyzerSampleOrdTest 8 with unchanged expectations, doc_comment_lint clean, and the three rendered strings still identical across the two suites.
ryanbr
added a commit
that referenced
this pull request
Aug 20, 2026
Follow-up to #1486. No behaviour change - this removes instrumentation that has finished its job. The shadow de-dup block ran 13 full passes over a night's R-R rows, six of them collapseOverCount, and each collapse SORTS the night's ~50-70k intervals. It runs for EVERY night of an affected strap, and analyzeRecent re-scores ~21 days every 15 minutes, so the phones least able to spare the work were paying the most for instrumentation that feeds no stored value. The same-second TOLERANCE SWEEP (20/34/60) was sizing a fix already ruled out: every affected night reads crossSecondOverCount, so no same-second tolerance can reach duplicates that straddle the boundary. `xsec` - the 40 ms collapse widened to a 1-second window - was documented from the start as a strict UPPER BOUND that over-merges real beats, kept only to size how far a cross-second collapse COULD get. It has now produced that number in the field: covXsec 0.80 with beatAccXsec 0.26, coverage driven BELOW 1.0 and accuracy collapsed, eating real beats exactly as its own comment predicted. The measurement succeeded, so it can stop running. What survives is the honest floor (`ex`, exact duplicates only, provably no real-beat loss) and the incumbent candidate (`dd`, 40 ms same-second). The delivery histogram from #1486 supersedes what both retired measurements reached for, at one pass instead of nine. 13 passes per night became 6; six sorts became two. Parity verified call-for-call rather than by reading the diff: eight analyzer calls in the same order on both platforms, the nine surviving `hrv dedup` fields matching name for name, and the emitted fragment byte-identical. That check mattered - an unused local on Kotlin compiles with only a warning, so a leftover accEx would have kept costing a full pass while looking clean. Also verified before deleting: nothing anywhere consumes `hrv sweep` or the xsec fields, and no orphaned variables survive on either platform. Verification: compileFullDebugKotlin clean, full Android unit suite green, doc_comment_lint clean.
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
1 participant
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.
Instrumentation for #1331 / #1008 / #1118 / #1451 — which are one defect.
Why a sample isn't enough
ordrestarts at 0 on every delivery, so two rows on one second both carryingord 0came from two different offloads writing the same wall second. Today's 5/MG log (#1451) shows exactly that:But
densestSecondWindowSampleonly dumps the 5–8 seconds around the densest one. That's a sample, not a measurement — and the fix turns on a question it cannot answer:If it's the former, the fix belongs at ingest and this sizes it. If it isn't, we were about to write the wrong fix.
The line
Read-side only — no new capture, no write-path change. It lights up on data already stored the moment someone opens the app on a build carrying it.
Two things it deliberately refuses to flatter
Both found on re-review, both cases where a convenient denominator would have argued against the mechanism:
multiRowsdivides by attributable rows (those carrying anord), not by every row. Dividing by the total would let a night that half-predates theordcolumn read artificially benign — precisely the conclusion this exists to prevent.secsNoStartcounts seconds carrying rows but noord 0row at all. That's reachable: the primary key absorbs a cross-batch exact duplicate, and the row it drops can be the delivery's first on that second. Reporting it stopssecsquietly shrinking the denominator underneathmultiSec.Percentages are integer half-up on both platforms, so a tie can't render differently per platform (the #1473 lesson).
Verification
ordcase.compileFullDebugKotlinclean, full Android unit suite green,HrvAnalyzerSampleOrdTest7,doc_comment_lintclean.StrandAnalyticsneeds macOS, so its suite runs in CI rather than locally.No behaviour change: nothing here feeds a stored value, a gate or a score.