Efficiently locate read messages in "unreads" state #4684
Labels
Comments
gnprice
added
a-redux
a-data-sync
gnprice
added a commit
to gnprice/zulip-mobile
that referenced
this issue
Apr 20, 2021
This reverts commit 8dc3fe6. This optimization has a bug: to locate a message in the unreads state, it looks it up in the messages state, but the messages state may be far less complete than the unreads state. (Which is in fact the point of the server feature that gives us the initial data to populate the unreads state.) So the message may be absent in `state.messages`, in which case we'll fail to remove it from `state.unread`. The symptom is that if you have the app running, then read some messages from the webapp or another client, they won't disappear from the unreads view. For now, just revert this optimization. We're still in a better shape than we were with the old data structures. Fundamentally we have all the data we need for this optimization -- if the message ID is in the unreads state, then we have the data of where in the unreads state it belongs -- and just need a data structure where that's indexed by message ID. I've filed zulip#4684 for doing that. But fix the bug first. See discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/syncing.20read.2Funread.20state/near/1166265 While here, also fix a small bug in the `updateAllAndPrune` helper from before the reverted commit: add a return after the `delete` call. The existing tests catch this; I'd refactored that function in a revision shortly before merge, and apparently didn't retest the intermediate commits in the branch. Reported-by: Wesley Aptekar-Cassels <[email protected]>
gnprice
added a commit
to gnprice/zulip-mobile
that referenced
this issue
Apr 20, 2021
Doing this from `state.messages` (at least as `state.messages` currently is) was buggy. Link instead to the issue about doing this optimization in a correct way.
Gautam-Arora24
pushed a commit
to Gautam-Arora24/zulip-mobile
that referenced
this issue
Apr 21, 2021
This reverts commit 8dc3fe6. This optimization has a bug: to locate a message in the unreads state, it looks it up in the messages state, but the messages state may be far less complete than the unreads state. (Which is in fact the point of the server feature that gives us the initial data to populate the unreads state.) So the message may be absent in `state.messages`, in which case we'll fail to remove it from `state.unread`. The symptom is that if you have the app running, then read some messages from the webapp or another client, they won't disappear from the unreads view. For now, just revert this optimization. We're still in a better shape than we were with the old data structures. Fundamentally we have all the data we need for this optimization -- if the message ID is in the unreads state, then we have the data of where in the unreads state it belongs -- and just need a data structure where that's indexed by message ID. I've filed zulip#4684 for doing that. But fix the bug first. See discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/syncing.20read.2Funread.20state/near/1166265 While here, also fix a small bug in the `updateAllAndPrune` helper from before the reverted commit: add a return after the `delete` call. The existing tests catch this; I'd refactored that function in a revision shortly before merge, and apparently didn't retest the intermediate commits in the branch. Reported-by: Wesley Aptekar-Cassels <[email protected]>
Gautam-Arora24
pushed a commit
to Gautam-Arora24/zulip-mobile
that referenced
this issue
Apr 21, 2021
Doing this from `state.messages` (at least as `state.messages` currently is) was buggy. Link instead to the issue about doing this optimization in a correct way.
|
In the webapp, |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is a followup to #4438.
One of the optimizations in #4448, which closed that issue, was that when we learn a message has been read, we'd efficiently look up the relevant bit of the unreads state and delete it there. Previously, we'd been searching through the whole data structure to find each newly-read message.
As implemented, this optimization had a bug: it relied on
state.messagesto tell it the conversation the message was in. Andstate.messagescan be far less complete thanstate.unread-- in fact that's the point of the server feature that initializesstate.unread. So we'd fail to find the message, andstate.unreadwould keep recording it as read. @WesleyAC discovered this and bisected it to the relevant commit.For an immediate fix, I'm about to revert that optimization. (That'll still leave us faster than with the old data structures.) But fundamentally we have the data we need to do it correctly: if a message is in the unreads state, then we know where in the unreads state it belongs, and we just need that information indexed by message ID. So we should build that data structure, and use it to restore this optimization.
Effectively that data structure will mean there's a small amount of per-message data we have on potentially a large number of messages, while we have a full
Messageobject -- notably, the message content -- on typically a much smaller number of messages.Design discussion in chat.
The text was updated successfully, but these errors were encountered: