Repository navigation
perf: sweep the HRV windows instead of re-filtering per window - #2560
Merged
Merged
Conversation
`sessionHrvWindows` re-filtered the whole R-R segment once per five-minute window. The CONTRACT above it already guarantees ts-sorted input, so each window's beats are a contiguous run and one advancing index does the same job. Both platforms, since Android carried the identical rescan. Measured rather than asserted, and smaller than the operation count suggests: on a 27,879-beat night, 0.90 ms to 0.03 ms, about 28x. Across the five call sites in AnalyticsEngine and the twenty-one days analyzeRecent covers, 94 ms to 3 ms per pass. Stating that plainly because the first estimate was misleading. Counting predicate evaluations gives order 10^8 per pass, which sounds like seconds; Swift's filter over a small struct is fast enough that the real figure is about ninety milliseconds. This is a clean asymptotic fix, not the reason a cold analyzeRecent prep takes sixty-eight seconds. That remains unexplained and the log points elsewhere, at the HR reads. Output is unchanged, and that is the part worth the effort, because this function produces the HRV that lands in the daily row, the sleep-session cache, the Health card and the baseline later nights are scored against. So both platforms get an ORACLE rather than a unit test: a reference implementation of the ORIGINAL bucketing, verified line by line against the pre-change source rather than from memory, feeding the same cleaning and RMSSD the production path uses, compared against the real function over 480 nights covering dense, sparse, duplicate-timestamp, tail-loaded, boundary and empty input. Both compare exactly rather than with a tolerance: identical bucketing through identical code must give bit-identical doubles, and a tolerance would quietly accept a real difference in a shipped number. The Swift oracle matters most and is the one that nearly did not get written. Swift is the reference implementation, and the GRDB link requirements mean this package cannot run on a plain Linux host, so that half would otherwise have rested on a throwaway check that ran once and no longer exists.
5 tasks done
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.
Summary
SleepStager.sessionHrvWindowsre-filtered the whole R-R segment once per five-minute window:The CONTRACT directly above it already guarantees ts-sorted input, so each window's beats are a contiguous run and one advancing index does the same job. Both platforms, since Android carried the identical rescan.
Found while reviewing the concurrency sample in @pipiche38's #2521.
The measurement, including where my own first estimate was wrong
On a 27,879-beat night, the size the reporting install actually banks: 0.90 ms to 0.03 ms, about 28x. Across the five
AnalyticsEnginecall sites and the twenty-one daysanalyzeRecentcovers: 94 ms to 3 ms per pass.I originally characterised this from operation counts, order 10^8 per pass, which sounds like seconds. Swift's
filterover a small struct is fast enough that the real figure is about ninety milliseconds. So this is a clean asymptotic fix worth having on its own terms, and it is not why a coldanalyzeRecentprep takes sixty-eight seconds. That remains unexplained; the strap log points at the HR reads (hr[read=1654472 served=2001295]).Why both platforms get an oracle rather than a unit test
This function produces the HRV that lands in the daily row, the sleep-session cache, the Health card, and the baseline later nights are scored against. The claim being made is byte-identical output, so that is what gets tested.
Each oracle re-implements the original bucketing straight from the rule, then hands each bucket to the same cleaning and RMSSD the production path uses, and compares whole windows against the real function over 480 nights: dense ~1Hz, sparse with gaps, duplicate same-second timestamps (which the contract warns about), everything crammed into the final window, a single beat on a boundary, and empty, across eight durations and ten seeds. Both assert the 480 cases actually ran, so the loop cannot silently do nothing.
Two details that decide whether an oracle is worth anything:
The reference was verified against the deleted source, line by line via
git show HEAD:, not from memory. A green oracle checking against a misremembered reference is worse than no oracle, because it manufactures confidence exactly where the risk is.Both compare exactly, not with a tolerance. Identical bucketing through identical code must give bit-identical doubles. A tolerance would quietly accept a real difference in a shipped number.
The Swift oracle is the one that matters
It nearly did not get written. The Swift check started as a throwaway binary that ran once and no longer exists, while Kotlin got a permanent test, which is the wrong way round: Swift is the reference implementation, and the GRDB link requirements mean this package cannot run on a plain Linux host. The half with the weakest standing verification had the greatest need for it.
Verification
./gradlew :app:testFullDebugUnitTest --no-build-cache --rerun-tasks: 767 suites, 6520 tests, 0 failures, result XML checked fresh against the clock. The Kotlin oracle passes 480/480../gradlew :app:lintVitalFullRelease -PstagingRelease: clean.python3 Tools/i18n_audit.py --ci origin/main: 0.python3 Tools/doc_comment_lint.py: clean.swift-packages.yml(macOS, path-filtered toPackages/**), which is its first real execution: this host fails to link the GRDB-backed package, asdocs/BUILD.mddescribes.