Repository navigation
perf(mobile): evict verified-profile interest in one pass - #9007
Conversation
#8931 replaced the per-eviction scan with `keys.first`, but each new iterator over a Dart map skips the slots earlier removals left behind, so one track call that overflows by E subjects still costs about E^2/2 slot checks. Eviction now takes the overflow from one iteration. The eviction test counted yielded keys, which `keys.first` keeps at one per eviction, so it could not see the quadratic cost. It now counts iterations started over the interest map and expects exactly one; the merged loop starts 3,276. Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Tom Brow <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
the
codex-security-review-current
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
No blockers. I'm leaving this as a comment, not an approval.
- Eviction matches the old loop.
_wanted.keys.take(overflow).toList()snapshots the oldestoverflowkeys in insertion order before anything is removed, so there's no concurrent modification, and it evicts the same subjects thekeys.firstloop did.added = truestill fires only when something was evicted. - The test now measures the right thing.
_CountingMapoverrideslength,isEmpty,containsKey,[],[]=andremove, so onlykeys(andentries/valuesviaMapBase) count as an iteration. Thesync*getter counts when iteration starts, sokeys.firstper eviction counts 3,276, which is the "Actual: <3276>" in the falsification note. The pass is exactly 1. - CI on
210c28d: 22 passed, 31 skipped, none failing.
Summary
Follow-up to #8931, from Wes's review note.
#8931 replaced the per-eviction scan in
VerifiedProfilesNotifier.trackwithkeys.first. That was much cheaper than main, but not constant time: removing from a Dart map leaves deleted slots, and each newkeysiterator walks past them. Onetrackcall that overflows the 1,024-subject cap by E subjects still costs about E²/2 slot checks. Wes measured ~3 ms at 4,300 subjects, 62 ms at 8,600 and 205 ms at 17,200.Eviction now takes the overflow from a single iteration (
keys.take(overflow).toList()), so it's linear. Which subjects are kept doesn't change: still the 1,024 most recently inserted.The #8931 eviction test counted keys yielded by the map, which
keys.firstkeeps at one per eviction, so it couldn't fail on the quadratic path. It now counts iterations started over the interest map and expects exactly one pertrackcall. Main's scan and the mergedkeys.firstloop both start one per eviction.Testing
Falsification: with the merged
keys.firstloop restored, the new test fails (Expected: <1>, Actual: <3276>). With this change it passes.flutter test test/shared/verified_profiles/: 31 passed. The existing "keeps the 1,024 most recently tracked subjects" journey still passes, so retention is unchanged.just mobile-check: clean.just mobile-test: 4,227 Flutter tests passed, 40 skipped, plus 4 push and 27 crypto tests.Automated app run: installed this build on an iOS 27 simulator (iPhone 17 Pro) signed in to a Builderlab community, opened the inbox, a channel timeline with images and the image viewer, and the channel details member list, which all track verified-profile subjects. No errors. That community is small (30 members), so the overflow path itself is covered by the unit tests, not the app run.
Agent review: an independent agent review found no blockers.
Human test: waived under the mobile testing exception; the automated app run above replaces it.
Not covered: Android, and no new device profile (Wes's AOT numbers above are the cost model; this removes the E² term).
buzz-review-completed
🤖 Generated with Claude Code