perf(reactions): bound reaction target search and bulk reparse - #854
Open
octoshrimpy wants to merge 1 commit into
Open
perf(reactions): bound reaction target search and bulk reparse#854octoshrimpy wants to merge 1 commit into
octoshrimpy wants to merge 1 commit into
Conversation
Fixes the ANR in #840, reported as "parsing emoji reactions" hanging after a first sync. Three costs compounded, all inside a single Realm transaction: - The bulk reparse cleared isEmojiReaction by walking every message in the database and writing the field whether or not it was set. Query the flagged messages instead, which on a typical history is a handful rather than tens of thousands. - findTargetMessage loaded every message in the thread and ran a regex over each one, and it runs once per reaction. That product is quadratic across an imported history. Bound it: a reaction can only target a message that already existed, so filter by date, and cap the scan at the 200 most recent candidates. A reaction refers to something the sender can still see, so a target further back is not plausible. - An untruncated quote, the common case, now matches natively in Realm on the body column instead of materialising each candidate's text through getText(), which walks MMS parts. MMS still falls through to the scan, since its text lives in parts rather than body. The date filter allows a minute of slack, because clock differences between two devices can timestamp a reaction fractionally before the message it targets. Also fixes the progress bar, which counted only messages that turned out to be reactions while max counted every message examined. Progress could therefore never reach max, so the bar never completed and the final callback never fired. reactionDate is optional on the interface so existing callers keep compiling; all three in-tree callers now pass it.
There was a problem hiding this comment.
Pull request overview
This PR addresses an ANR reported during bulk “parsing emoji reactions” by reducing the amount of work done inside a Realm transaction and preventing quadratic scans when resolving reaction targets.
Changes:
- Extends
findTargetMessagewith an optionalreactionDateto bound target searches to plausible candidates. - Optimizes reaction target lookup by adding a Realm-side exact-body match for common cases, applying a date upper-bound, and capping candidate scans.
- Reduces bulk reparse write volume (clear only flagged messages) and fixes progress reporting to reach completion.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| domain/src/main/java/com/moez/QKSMS/repository/EmojiReactionRepository.kt | Adds reactionDate parameter and documents bounded target searching. |
| data/src/main/java/com/moez/QKSMS/repository/SyncRepositoryImpl.kt | Passes reaction message date into findTargetMessage during sync reparse. |
| data/src/main/java/com/moez/QKSMS/repository/MessageRepositoryImpl.kt | Passes reaction message date into findTargetMessage for incoming messages. |
| data/src/main/java/com/moez/QKSMS/repository/EmojiReactionRepositoryImpl.kt | Implements bounded/capped target lookup, reduces full-table writes during reparse, and fixes progress accounting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+32
| /** | ||
| * [reactionDate] bounds the search to messages that already existed when the reaction was | ||
| * sent. Omitting it searches the whole thread, which is slow on a long history. | ||
| */ |
Comment on lines
193
to
+205
| override fun findTargetMessage( | ||
| threadId: Long, | ||
| originalMessageText: String, | ||
| realm: Realm | ||
| realm: Realm, | ||
| reactionDate: Long?, | ||
| ): Message? { | ||
| val startTime = System.currentTimeMillis() | ||
| val messages = realm.where(Message::class.java) | ||
| // Bound the search. A reaction can only target a message that already existed, and only | ||
| // recent ones are plausible targets. Unbounded, this loaded every message in the thread and | ||
| // ran a regex over each -- once per reaction. Across an imported history that product is | ||
| // the ANR in #840. | ||
| // Allow slack on the date: clock differences between two devices can timestamp a reaction | ||
| // marginally before the message it targets, and dropping the real target is worse than | ||
| // scanning a few extra. |
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.
Fixes #840.
Fixes the ANR in #840, reported as "parsing emoji reactions" hanging after a first sync. Three costs compounded, all inside a single Realm transaction:
The bulk reparse cleared isEmojiReaction by walking every message in the database and writing the field whether or not it was set. Query the flagged messages instead, which on a typical history is a handful rather than tens of thousands.
findTargetMessage loaded every message in the thread and ran a regex over each one, and it runs once per reaction. That product is quadratic across an imported history. Bound it: a reaction can only target a message that already existed, so filter by date, and cap the scan at the 200 most recent candidates. A reaction refers to something the sender can still see, so a target further back is not plausible.
An untruncated quote, the common case, now matches natively in Realm on the body column instead of materialising each candidate's text through getText(), which walks MMS parts. MMS still falls through to the scan, since its text lives in parts rather than body.
The date filter allows a minute of slack, because clock differences between two devices can timestamp a reaction fractionally before the message it targets.
Also fixes the progress bar, which counted only messages that turned out to be reactions while max counted every message examined. Progress could therefore never reach max, so the bar never completed and the final callback never fired.
reactionDate is optional on the interface so existing callers keep compiling; all three in-tree callers now pass it.
Symptom. The ANR is a broadcast timeout on
SMS_DELIVERat ~118% CPU — an incoming SMS could not be serviced because reaction reparsing was saturating the process.Scope. Deliberately minimal and independent of both in-flight reaction branches, so it can land under either.
reactionDateis optional on the interface, so #846 keeps compiling across a rebase.Testing.
:data:compileDebugKotlinpasses. Not unit tested — the project has no test source sets, and this is Realm-query behaviour that needs a device. Wants a manual check: fresh install, sync a large history, confirm no ANR and that the parsing progress bar reaches completion.