From 61b1c10314564c8ec35d9ad261bd013075b6ef99 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Tue, 12 Jan 2021 17:52:05 -0800
Subject: [PATCH 01/14] docs/style: Document gotcha and workaround on Immutable
 types.

A couple of bugs slipped by me at first in draft changes for
converting the "unreads" model to Immutable, because I wasn't
anticipating this.
---
 docs/style.md | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/docs/style.md b/docs/style.md
index 4fabcad636d..9a9b37df7af 100644
--- a/docs/style.md
+++ b/docs/style.md
@@ -275,7 +275,7 @@ pick just one, and that's the one we use.
 [gh-close-issue-keywords]: https://help.github.com/en/github/managing-your-work-on-github/closing-issues-using-keywords
 
 
-## JavaScript and Flow
+## JavaScript, Flow, JS libraries
 
 **Use `invariant` for runtime assertions the type-checker can use**:
 If there's a fact you're sure is true at a certain point in the code,
@@ -294,6 +294,21 @@ definitely mean a bug within our own zulip-mobile codebase.
 [flow-invariant-pseudodocs]: https://github.com/facebook/flow/issues/6052
 
 
+**Always provide a type when writing an empty `Immutable` value**:
+Whenever you create an empty `Immutable.Map`, `Immutable.List`, or
+so on, specify the intended type explicitly.  For example:
+```js
+const map: Immutable.Map<number, string> = Immutable.Map(); // good
+
+const map = Immutable.Map(); // BAD -- don't do
+```
+
+This is essential in order to get effective type-checking of the
+code that uses the new collection.  (It's not clear if this is a bug
+in Flow, or a design limitation of Flow, or an issue in the Flow types
+provided by Immutable.js, or some combination.)
+
+
 ## Internal to Zulip and our codebase
 
 ### Zulip API bindings

From 4ca8aec6dfb91f12a3d8784cbfb90375bfa1f278 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Mon, 11 Jan 2021 20:20:16 -0800
Subject: [PATCH 02/14] unread [nfc]: Rename an old "direct" selector as being
 legacy.

We're going to switch the data structure we use for this to something
that's efficient to keep updated (#4438).  The new one will be just
as well suited for code that consumes this data structure as the old
-- in fact better for some of them -- so we'll have the consumers
switch to using the new data structure, too.  That means the selector
that provides the old data structure is going away.

As a first step, we rename the old selector to a "legacy" name,
making room for a selector providing the new data structure to
take its name.
---
 src/topics/topicSelectors.js  |  4 ++--
 src/unread/unreadModel.js     |  3 ++-
 src/unread/unreadSelectors.js | 13 +++++++++----
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/src/topics/topicSelectors.js b/src/topics/topicSelectors.js
index f25b482b22c..cd89b34b2cf 100644
--- a/src/topics/topicSelectors.js
+++ b/src/topics/topicSelectors.js
@@ -10,7 +10,7 @@ import type {
   TopicsState,
 } from '../types';
 import { getMute, getStreams, getTopics } from '../directSelectors';
-import { getUnreadStreams } from '../unread/unreadModel';
+import { getUnreadStreamsLegacy } from '../unread/unreadModel';
 import { getShownMessagesForNarrow } from '../chat/narrowsSelectors';
 import { getStreamsById } from '../subscriptions/subscriptionSelectors';
 import { NULL_ARRAY } from '../nullObjects';
@@ -44,7 +44,7 @@ export const getTopicsForStream: Selector<?(TopicExtended[]), number> = createSe
   (state, streamId) => getTopics(state)[streamId],
   state => getMute(state),
   (state, streamId) => getStreamsById(state).get(streamId),
-  state => getUnreadStreams(state),
+  state => getUnreadStreamsLegacy(state),
   (topicList, mute, stream, unreadStreams) => {
     if (!topicList || !stream) {
       return undefined;
diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js
index 9c8878b1c1d..908008e3ab1 100644
--- a/src/unread/unreadModel.js
+++ b/src/unread/unreadModel.js
@@ -14,7 +14,8 @@ import unreadPmsReducer from './unreadPmsReducer';
 import unreadHuddlesReducer from './unreadHuddlesReducer';
 import unreadMentionsReducer from './unreadMentionsReducer';
 
-export const getUnreadStreams = (state: GlobalState): UnreadStreamsState => state.unread.streams;
+export const getUnreadStreamsLegacy = (state: GlobalState): UnreadStreamsState =>
+  state.unread.streams;
 
 export const getUnreadPms = (state: GlobalState): UnreadPmsState => state.unread.pms;
 
diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js
index 66569154906..bc8bf574166 100644
--- a/src/unread/unreadSelectors.js
+++ b/src/unread/unreadSelectors.js
@@ -10,11 +10,16 @@ import { isTopicMuted } from '../utils/message';
 import { caseNarrow } from '../utils/narrow';
 import { NULL_SUBSCRIPTION } from '../nullObjects';
 import { pmUnreadsKeyFromPmKeyIds } from '../utils/recipient';
-import { getUnreadStreams, getUnreadPms, getUnreadHuddles, getUnreadMentions } from './unreadModel';
+import {
+  getUnreadStreamsLegacy,
+  getUnreadPms,
+  getUnreadHuddles,
+  getUnreadMentions,
+} from './unreadModel';
 
 /** The number of unreads in each stream, excluding muted topics, by stream ID. */
 export const getUnreadByStream: Selector<{| [number]: number |}> = createSelector(
-  getUnreadStreams,
+  getUnreadStreamsLegacy,
   getSubscriptionsById,
   getMute,
   (unreadStreams, subscriptionsById, mute) => {
@@ -126,7 +131,7 @@ export const getUnreadTotal: Selector<number> = createSelector(
 /** Helper for getUnreadStreamsAndTopicsSansMuted; see there. */
 export const getUnreadStreamsAndTopics: Selector<UnreadStreamItem[]> = createSelector(
   getSubscriptionsById,
-  getUnreadStreams,
+  getUnreadStreamsLegacy,
   getMute,
   (subscriptionsById, unreadStreams, mute) => {
     const totals = new Map();
@@ -218,7 +223,7 @@ export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
   state => getStreams(state),
   state => getOwnUserId(state),
   state => getUnreadTotal(state),
-  state => getUnreadStreams(state),
+  state => getUnreadStreamsLegacy(state),
   state => getUnreadHuddles(state),
   state => getUnreadPms(state),
   state => getMute(state),

From 1c2225babb55fa728e96143b679ad70cc591e7e5 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Mon, 11 Jan 2021 20:27:10 -0800
Subject: [PATCH 03/14] unread: Add new getUnreadStreams selector.

This provides a data structure that's just like the one we're going
to switch to maintaining as the state internally.  We'll migrate
consumers to use this one, and then we can change out the state and
reducer underneath it.

As we migrate consumers to this selector, it may initially cause a
certain performance regression: when the "unread" data structure gets
updated, we'll promptly be making this (reformatted) copy of it, as
the selectors that use it go to look for the new data.  But that extra
work will then get eliminated when we switch to this being the data
structure that we maintain in the first place.
---
 src/unread/unreadModel.js | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js
index 908008e3ab1..e30b1e6704f 100644
--- a/src/unread/unreadModel.js
+++ b/src/unread/unreadModel.js
@@ -1,4 +1,6 @@
 /* @flow strict-local */
+import Immutable from 'immutable';
+import { createSelector } from 'reselect';
 
 import type { Action } from '../actionTypes';
 import type {
@@ -9,6 +11,7 @@ import type {
   UnreadStreamsState,
 } from './unreadModelTypes';
 import type { GlobalState } from '../reduxTypes';
+import type { Selector } from '../types';
 import unreadStreamsReducer from './unreadStreamsReducer';
 import unreadPmsReducer from './unreadPmsReducer';
 import unreadHuddlesReducer from './unreadHuddlesReducer';
@@ -17,6 +20,26 @@ import unreadMentionsReducer from './unreadMentionsReducer';
 export const getUnreadStreamsLegacy = (state: GlobalState): UnreadStreamsState =>
   state.unread.streams;
 
+export const getUnreadStreams: Selector<
+  Immutable.Map<number, Immutable.Map<string, Immutable.List<number>>>,
+> = createSelector(
+  getUnreadStreamsLegacy,
+  data => {
+    // prettier-ignore
+    // This is an example of the style-guide rule "Always provide a type
+    // when writing an empty `Immutable` value".  Without the explicit type,
+    // `result` gets inferred as `Immutable.Map<number, empty>`, and then
+    // e.g. the `setIn` call could be completely wrong and the type-checker
+    // wouldn't notice.
+    const result: Immutable.Map<number, Immutable.Map<string, Immutable.List<number>>> =
+      Immutable.Map().asMutable();
+    for (const { stream_id, topic, unread_message_ids } of data) {
+      result.setIn([stream_id, topic], Immutable.List(unread_message_ids));
+    }
+    return result.asImmutable();
+  },
+);
+
 export const getUnreadPms = (state: GlobalState): UnreadPmsState => state.unread.pms;
 
 export const getUnreadHuddles = (state: GlobalState): UnreadHuddlesState => state.unread.huddles;

From 9d2445d66e8cbbeb406fa49eb5f218ee60eb1a5a Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Mon, 11 Jan 2021 20:32:37 -0800
Subject: [PATCH 04/14] unread [nfc]: Convert getUnreadStreams use in
 getUnreadByStream.

The logic gets slightly simpler because we get to do our whole
computation for each stream all at once before moving on to the next.
---
 src/unread/unreadSelectors.js | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js
index bc8bf574166..07b500137cf 100644
--- a/src/unread/unreadSelectors.js
+++ b/src/unread/unreadSelectors.js
@@ -15,26 +15,28 @@ import {
   getUnreadPms,
   getUnreadHuddles,
   getUnreadMentions,
+  getUnreadStreams,
 } from './unreadModel';
 
 /** The number of unreads in each stream, excluding muted topics, by stream ID. */
 export const getUnreadByStream: Selector<{| [number]: number |}> = createSelector(
-  getUnreadStreamsLegacy,
+  getUnreadStreams,
   getSubscriptionsById,
   getMute,
   (unreadStreams, subscriptionsById, mute) => {
     const totals = ({}: {| [number]: number |});
-    unreadStreams.forEach(stream => {
-      if (!totals[stream.stream_id]) {
-        totals[stream.stream_id] = 0;
+    for (const [streamId, streamData] of unreadStreams.entries()) {
+      let total = 0;
+      for (const [topic, msgIds] of streamData) {
+        const isMuted = isTopicMuted(
+          (subscriptionsById.get(streamId) || NULL_SUBSCRIPTION).name,
+          topic,
+          mute,
+        );
+        total += isMuted ? 0 : msgIds.size;
       }
-      const isMuted = isTopicMuted(
-        (subscriptionsById.get(stream.stream_id) || NULL_SUBSCRIPTION).name,
-        stream.topic,
-        mute,
-      );
-      totals[stream.stream_id] += isMuted ? 0 : stream.unread_message_ids.length;
-    });
+      totals[streamId] = total;
+    }
     return totals;
   },
 );

From 3eec017022f36db417933035ecf7f4c901ab0e39 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Mon, 11 Jan 2021 20:40:10 -0800
Subject: [PATCH 05/14] unread [nfc]: Convert getUnreadStreams use in
 getUnreadStreamsAndTopics.

This one also gets slightly simpler because we now get to handle
each entire stream all at once in turn.
---
 src/unread/unreadSelectors.js | 57 +++++++++++++++++------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js
index 07b500137cf..ff8717f6984 100644
--- a/src/unread/unreadSelectors.js
+++ b/src/unread/unreadSelectors.js
@@ -133,42 +133,41 @@ export const getUnreadTotal: Selector<number> = createSelector(
 /** Helper for getUnreadStreamsAndTopicsSansMuted; see there. */
 export const getUnreadStreamsAndTopics: Selector<UnreadStreamItem[]> = createSelector(
   getSubscriptionsById,
-  getUnreadStreamsLegacy,
+  getUnreadStreams,
   getMute,
   (subscriptionsById, unreadStreams, mute) => {
     const totals = new Map();
-    unreadStreams.forEach(stream => {
+    for (const [streamId, streamData] of unreadStreams.entries()) {
       const { name, color, in_home_view, invite_only, pin_to_top } =
-        subscriptionsById.get(stream.stream_id) || NULL_SUBSCRIPTION;
+        subscriptionsById.get(streamId) || NULL_SUBSCRIPTION;
 
-      let total = totals.get(stream.stream_id);
-      if (!total) {
-        total = {
-          key: `stream:${name}`,
-          streamName: name,
-          isMuted: !in_home_view,
-          isPrivate: invite_only,
-          isPinned: pin_to_top,
-          color,
-          unread: 0,
-          data: [],
-        };
-        totals.set(stream.stream_id, total);
-      }
+      const total = {
+        key: `stream:${name}`,
+        streamName: name,
+        isMuted: !in_home_view,
+        isPrivate: invite_only,
+        isPinned: pin_to_top,
+        color,
+        unread: 0,
+        data: [],
+      };
+      totals.set(streamId, total);
 
-      const isMuted = !mute.every(x => x[0] !== name || x[1] !== stream.topic);
-      if (!isMuted) {
-        total.unread += stream.unread_message_ids.length;
-      }
+      for (const [topic, msgIds] of streamData) {
+        const isMuted = !mute.every(x => x[0] !== name || x[1] !== topic);
+        if (!isMuted) {
+          total.unread += msgIds.size;
+        }
 
-      total.data.push({
-        key: stream.topic,
-        topic: stream.topic,
-        unread: stream.unread_message_ids.length,
-        lastUnreadMsgId: stream.unread_message_ids[stream.unread_message_ids.length - 1],
-        isMuted,
-      });
-    });
+        total.data.push({
+          key: topic,
+          topic,
+          unread: msgIds.size,
+          lastUnreadMsgId: msgIds.last(),
+          isMuted,
+        });
+      }
+    }
 
     const sortedStreams = Array.from(totals.values())
       .sort((a, b) => caseInsensitiveCompareFunc(a.streamName, b.streamName))

From fc1532e10b8aea5d72ac2457e8164afa08008f8f Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Mon, 11 Jan 2021 20:47:39 -0800
Subject: [PATCH 06/14] unread: Convert getUnreadStreams use in
 getUnreadCountForNarrow.

This gets rather more efficient, because we no longer need to scan
through the entire data structure and can instead go straight to the
particular stream, or stream and topic, that we're interested in.
---
 src/unread/unreadSelectors.js | 36 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js
index ff8717f6984..d2e28b2c867 100644
--- a/src/unread/unreadSelectors.js
+++ b/src/unread/unreadSelectors.js
@@ -10,13 +10,7 @@ import { isTopicMuted } from '../utils/message';
 import { caseNarrow } from '../utils/narrow';
 import { NULL_SUBSCRIPTION } from '../nullObjects';
 import { pmUnreadsKeyFromPmKeyIds } from '../utils/recipient';
-import {
-  getUnreadStreamsLegacy,
-  getUnreadPms,
-  getUnreadHuddles,
-  getUnreadMentions,
-  getUnreadStreams,
-} from './unreadModel';
+import { getUnreadPms, getUnreadHuddles, getUnreadMentions, getUnreadStreams } from './unreadModel';
 
 /** The number of unreads in each stream, excluding muted topics, by stream ID. */
 export const getUnreadByStream: Selector<{| [number]: number |}> = createSelector(
@@ -224,14 +218,12 @@ export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
   state => getStreams(state),
   state => getOwnUserId(state),
   state => getUnreadTotal(state),
-  state => getUnreadStreamsLegacy(state),
+  state => getUnreadStreams(state),
   state => getUnreadHuddles(state),
   state => getUnreadPms(state),
   state => getMute(state),
-  (narrow, streams, ownUserId, unreadTotal, unreadStreams, unreadHuddles, unreadPms, mute) => {
-    const sumLengths = unreads => unreads.reduce((sum, x) => sum + x.unread_message_ids.length, 0);
-
-    return caseNarrow(narrow, {
+  (narrow, streams, ownUserId, unreadTotal, unreadStreams, unreadHuddles, unreadPms, mute) =>
+    caseNarrow(narrow, {
       home: () => unreadTotal,
 
       stream: name => {
@@ -239,10 +231,15 @@ export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
         if (!stream) {
           return 0;
         }
-        return sumLengths(
-          unreadStreams.filter(
-            x => x.stream_id === stream.stream_id && !isTopicMuted(name, x.topic, mute),
-          ),
+        // prettier-ignore
+        return (
+          unreadStreams
+            .get(stream.stream_id)
+            ?.entrySeq()
+            .filterNot(([topic, _]) => isTopicMuted(name, topic, mute))
+            .map(([_, msgIds]) => msgIds.size)
+            .reduce((s, x) => s + x, 0)
+            ?? 0
         );
       },
 
@@ -251,9 +248,7 @@ export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
         if (!stream) {
           return 0;
         }
-        return sumLengths(
-          unreadStreams.filter(x => x.stream_id === stream.stream_id && x.topic === topic),
-        );
+        return unreadStreams.get(stream.stream_id)?.get(topic)?.size ?? 0;
       },
 
       pm: ids => {
@@ -283,6 +278,5 @@ export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
       // because we never use this selector for that narrow (because we
       // don't expose it as one you can narrow to in the UI.)
       allPrivate: () => 0,
-    });
-  },
+    }),
 );

From 2f7c0760402bda90e6a881575b8d0e3b5e4347f8 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Mon, 11 Jan 2021 20:53:03 -0800
Subject: [PATCH 07/14] unread: Convert unreadStreams use in
 getTopicsForStream.

Oof, this had been quadratic!  We would scan through the entire
unreads data structure, looking just for one particular entry...
again and again for each topic in turn.

Now we just go straight to the single entry for this particular topic.
Much better.
---
 src/topics/topicSelectors.js | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/src/topics/topicSelectors.js b/src/topics/topicSelectors.js
index cd89b34b2cf..7f0eabcba83 100644
--- a/src/topics/topicSelectors.js
+++ b/src/topics/topicSelectors.js
@@ -10,7 +10,7 @@ import type {
   TopicsState,
 } from '../types';
 import { getMute, getStreams, getTopics } from '../directSelectors';
-import { getUnreadStreamsLegacy } from '../unread/unreadModel';
+import { getUnreadStreams } from '../unread/unreadModel';
 import { getShownMessagesForNarrow } from '../chat/narrowsSelectors';
 import { getStreamsById } from '../subscriptions/subscriptionSelectors';
 import { NULL_ARRAY } from '../nullObjects';
@@ -44,7 +44,7 @@ export const getTopicsForStream: Selector<?(TopicExtended[]), number> = createSe
   (state, streamId) => getTopics(state)[streamId],
   state => getMute(state),
   (state, streamId) => getStreamsById(state).get(streamId),
-  state => getUnreadStreamsLegacy(state),
+  state => getUnreadStreams(state),
   (topicList, mute, stream, unreadStreams) => {
     if (!topicList || !stream) {
       return undefined;
@@ -52,15 +52,8 @@ export const getTopicsForStream: Selector<?(TopicExtended[]), number> = createSe
 
     return topicList.map(({ name, max_id }) => {
       const isMuted = !!mute.find(x => x[0] === stream.name && x[1] === name);
-      const unreadStream = unreadStreams.find(
-        x => x.stream_id === stream.stream_id && x.topic === name,
-      );
-      return {
-        name,
-        max_id,
-        isMuted,
-        unreadCount: unreadStream ? unreadStream.unread_message_ids.length : 0,
-      };
+      const unreadCount = unreadStreams.get(stream.stream_id)?.get(name)?.size ?? 0;
+      return { name, max_id, isMuted, unreadCount };
     });
   },
 );

From 7d164fd6fbdbee2e06d03f37c8eb8720c3500c07 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Mon, 11 Jan 2021 20:53:45 -0800
Subject: [PATCH 08/14] unread [nfc]: Cut getUnreadStreamsLegacy.

We've converted all its consumers to use the new `getUnreadStreams`
data structure instead... except for the `getUnreadStreams` selector
itself.  That one will simplify out when we switch to maintaining
the new data structure in Redux, coming next.
---
 src/unread/unreadModel.js | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js
index e30b1e6704f..cc4f9481e40 100644
--- a/src/unread/unreadModel.js
+++ b/src/unread/unreadModel.js
@@ -8,7 +8,6 @@ import type {
   UnreadPmsState,
   UnreadHuddlesState,
   UnreadMentionsState,
-  UnreadStreamsState,
 } from './unreadModelTypes';
 import type { GlobalState } from '../reduxTypes';
 import type { Selector } from '../types';
@@ -17,13 +16,10 @@ import unreadPmsReducer from './unreadPmsReducer';
 import unreadHuddlesReducer from './unreadHuddlesReducer';
 import unreadMentionsReducer from './unreadMentionsReducer';
 
-export const getUnreadStreamsLegacy = (state: GlobalState): UnreadStreamsState =>
-  state.unread.streams;
-
 export const getUnreadStreams: Selector<
   Immutable.Map<number, Immutable.Map<string, Immutable.List<number>>>,
 > = createSelector(
-  getUnreadStreamsLegacy,
+  state => state.unread.streams,
   data => {
     // prettier-ignore
     // This is an example of the style-guide rule "Always provide a type

From e3a2f94ccc0084ce0a24a2a1828054757ea77645 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Tue, 12 Jan 2021 18:22:31 -0800
Subject: [PATCH 09/14] unread tests: Delete a spurious test.

Getting the same message ID twice just shouldn't happen -- it'd be
a break in the contract of the server API.  If some server bug does
ever cause it to happen, and we don't notice and just include the
additional message ID here, nothing particularly bad will happen;
at worst a glitch where the unread counts are too high.

Meanwhile, maintaining this data structure is a hot path, so it
won't make sense for us to defensively check for such a server bug.
Take out the test that checks for it.
---
 src/unread/__tests__/unreadModel-test.js | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/src/unread/__tests__/unreadModel-test.js b/src/unread/__tests__/unreadModel-test.js
index 813feb6f04e..1716cddbb45 100644
--- a/src/unread/__tests__/unreadModel-test.js
+++ b/src/unread/__tests__/unreadModel-test.js
@@ -87,15 +87,6 @@ describe('stream substate', () => {
       ]));
     });
 
-    test('if message id already exists, do not mutate state', () => {
-      const state = reducer(
-        baseState,
-        action(eg.streamMessage({ id: 1, subject: 'some topic' })),
-        eg.plusReduxState,
-      );
-      expect(state).toBe(baseState);
-    });
-
     test('if message is not stream, return original state', () => {
       const state = reducer(baseState, action(eg.pmMessage({ id: 4 })), eg.plusReduxState);
       expect(state.streams).toBe(baseState.streams);

From 89cff7c24721776d3c9d9ff6ec94f1c32f89214c Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Mon, 11 Jan 2021 21:39:01 -0800
Subject: [PATCH 10/14] unread: Convert unread.streams to Immutable!

This makes us quite a bit faster at handling a message being marked
as read!  That's a frequent and latency-sensitive path -- whenever
the user is reading a conversation and sees some messages that were
unread, this is part of the path to updating the "N unreads" banner
at the top of their view.  Measurements described below.

As we convert the other parts of the unread state, we'll want to fold
their reducers into this file too, and in fact combine the logic.  No
need to have four copies of all this, most of which is the same.

While making this conversion, I notice that this reducer doesn't reset
state on LOGIN_SUCCESS like it does for LOGOUT and ACCOUNT_SWITCH.  In
general we're pretty consistent about resetting state on those latter
two, but many of our reducers do so on LOGIN_SUCCESS while many others
don't.  I've filed #4446 for fixing them all up to be consistent.

Performance measurements:

I made some manual performance measurements to evaluate this change
and the others in this series.  I used a test user with lots of
unreads on chat.zulip.org, on a few-years-old flagship phone: a
Pixel 2 XL running Android 10.  The test user has 50k unreads in this
data structure (that's the max the server will send in the initial
fetch), across about 3400 topics in 27 different streams.

Before this series, on visiting a topic with 1 unread, we'd spend
about 70ms in this reducer, which is a long time.  We'd spend 300ms in
total on dispatching the EVENT_UPDATE_MESSAGE_FLAGS action, including
the time spent in the reducer.  (The other time is probably some
combination of React re-rendering the components that use this data;
before that, our selectors that sit between those components and this
data recomputing their own results; and after that, React Native
applying the resulting updates to the underlying native components.
We don't yet have clear measurements to tell how much time those each
contribute.)

After this change, we spend about 30-50ms in the reducer, and a total
of 150-200ms in dispatch.  Still slow, but much improved!  We'll speed
this up further in upcoming commits.

For EVENT_NEW_MESSAGE, which is the other frequent update to this data
structure, not much changes: it was already "only" 4-9ms spent in this
reducer, which is too slow but far from our worst performance problem.
After this change, it's usually <=1ms (too small to measure with our
tools, until recently), and the longest I've seen is 3ms.  The total
dispatch time varies widely, like 70-200ms; not clear if it changed.

There is one performance regression: we now spend about 100ms here on
REALM_INIT, i.e. on handling the data from the initial fetch.
Previously that time was <=1ms; we just took the data straight off the
wire (well, the data we'd already deserialized from the JSON that came
off the wire), and now we have to copy it into our more
efficiently-usable data structure.  As is, that 100ms is already well
worth it: we save more than 100ms, of visible latency, every time the
user reads some unread messages.  But it's enough to be worth
optimizing too, and we'll do so later in this series.

Fixes-partly: #4438
---
 src/boot/store.js                        |   3 +
 src/unread/__tests__/unreadModel-test.js |  18 +--
 src/unread/unreadHelpers.js              |  23 +--
 src/unread/unreadModel.js                | 178 ++++++++++++++++++++---
 src/unread/unreadModelTypes.js           |   7 +-
 src/unread/unreadStreamsReducer.js       |  78 ----------
 6 files changed, 169 insertions(+), 138 deletions(-)
 delete mode 100644 src/unread/unreadStreamsReducer.js

diff --git a/src/boot/store.js b/src/boot/store.js
index c1f538d2941..8e823a5653d 100644
--- a/src/boot/store.js
+++ b/src/boot/store.js
@@ -260,6 +260,9 @@ const migrations: {| [string]: (GlobalState) => GlobalState |} = {
   // See `purge` call in src/third/redux-persist/persistStore.js.
   '24': dropCache,
 
+  // Convert `unread.streams` from over-the-wire array to `Immutable.Map`.
+  '25': dropCache,
+
   // TIP: When adding a migration, consider just using `dropCache`.
 };
 
diff --git a/src/unread/__tests__/unreadModel-test.js b/src/unread/__tests__/unreadModel-test.js
index 1716cddbb45..2c47db90c45 100644
--- a/src/unread/__tests__/unreadModel-test.js
+++ b/src/unread/__tests__/unreadModel-test.js
@@ -3,6 +3,7 @@ import Immutable from 'immutable';
 
 import { ACCOUNT_SWITCH, EVENT_UPDATE_MESSAGE_FLAGS } from '../../actionConstants';
 import { reducer } from '../unreadModel';
+import { type UnreadState } from '../unreadModelTypes';
 import * as eg from '../../__tests__/lib/exampleData';
 import { initialState, mkMessageAction } from './unread-testlib';
 
@@ -12,19 +13,10 @@ import { initialState, mkMessageAction } from './unread-testlib';
 // but this way simplifies the conversion from the old tests.
 describe('stream substate', () => {
   // Summarize the state, for convenient comparison to expectations.
-  // In particular, abstract away irrelevant details of the ordering of
-  // streams and topics in the data structure -- those should never matter
-  // to selectors, and in a better data structure they wouldn't exist in the
-  // first place.
-  const summary = state => {
-    // prettier-ignore
-    const result: Immutable.Map<number, Immutable.Map<string, number[]>> =
-      Immutable.Map().asMutable();
-    for (const { stream_id, topic, unread_message_ids } of state.streams) {
-      result.setIn([stream_id, topic], unread_message_ids);
-    }
-    return result.asImmutable();
-  };
+  // Specifically just turn the inner `Immutable.List`s into arrays,
+  // to shorten writing the expected data.
+  const summary = (state: UnreadState) =>
+    state.streams.map(perStream => perStream.map(perTopic => perTopic.toArray()));
 
   describe('ACCOUNT_SWITCH', () => {
     test('resets state to initial state', () => {
diff --git a/src/unread/unreadHelpers.js b/src/unread/unreadHelpers.js
index 72ae97fbf8f..1fa8635b4f2 100644
--- a/src/unread/unreadHelpers.js
+++ b/src/unread/unreadHelpers.js
@@ -1,5 +1,5 @@
 /* @flow strict-local */
-import type { HuddlesUnreadItem, PmsUnreadItem, StreamUnreadItem, UserId } from '../types';
+import type { HuddlesUnreadItem, PmsUnreadItem, UserId } from '../types';
 import { addItemsToArray, removeItemsFromArray, filterArray } from '../utils/immutability';
 
 type SomeUnreadItem = { unread_message_ids: number[], ... };
@@ -86,24 +86,3 @@ export const addItemsToHuddleArray = (
     },
   ];
 };
-
-export const addItemsToStreamArray = (
-  input: StreamUnreadItem[],
-  itemsToAdd: number[],
-  streamId: number,
-  topic: string,
-): StreamUnreadItem[] => {
-  const index = input.findIndex(s => s.stream_id === streamId && s.topic === topic);
-
-  if (index !== -1) {
-    return addItemsDeeply(input, itemsToAdd, index);
-  }
-  return [
-    ...input,
-    {
-      stream_id: streamId,
-      topic,
-      unread_message_ids: itemsToAdd,
-    },
-  ];
-};
diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js
index cc4f9481e40..f3090e6ef9c 100644
--- a/src/unread/unreadModel.js
+++ b/src/unread/unreadModel.js
@@ -1,40 +1,35 @@
 /* @flow strict-local */
 import Immutable from 'immutable';
-import { createSelector } from 'reselect';
 
 import type { Action } from '../actionTypes';
 import type {
   UnreadState,
+  UnreadStreamsState,
   UnreadPmsState,
   UnreadHuddlesState,
   UnreadMentionsState,
 } from './unreadModelTypes';
 import type { GlobalState } from '../reduxTypes';
-import type { Selector } from '../types';
-import unreadStreamsReducer from './unreadStreamsReducer';
 import unreadPmsReducer from './unreadPmsReducer';
 import unreadHuddlesReducer from './unreadHuddlesReducer';
 import unreadMentionsReducer from './unreadMentionsReducer';
+import {
+  ACCOUNT_SWITCH,
+  EVENT_MESSAGE_DELETE,
+  EVENT_NEW_MESSAGE,
+  EVENT_UPDATE_MESSAGE_FLAGS,
+  LOGOUT,
+  MESSAGE_FETCH_COMPLETE,
+  REALM_INIT,
+} from '../actionConstants';
+import { getOwnUserId } from '../users/userSelectors';
 
-export const getUnreadStreams: Selector<
-  Immutable.Map<number, Immutable.Map<string, Immutable.List<number>>>,
-> = createSelector(
-  state => state.unread.streams,
-  data => {
-    // prettier-ignore
-    // This is an example of the style-guide rule "Always provide a type
-    // when writing an empty `Immutable` value".  Without the explicit type,
-    // `result` gets inferred as `Immutable.Map<number, empty>`, and then
-    // e.g. the `setIn` call could be completely wrong and the type-checker
-    // wouldn't notice.
-    const result: Immutable.Map<number, Immutable.Map<string, Immutable.List<number>>> =
-      Immutable.Map().asMutable();
-    for (const { stream_id, topic, unread_message_ids } of data) {
-      result.setIn([stream_id, topic], Immutable.List(unread_message_ids));
-    }
-    return result.asImmutable();
-  },
-);
+//
+//
+// Selectors.
+//
+
+export const getUnreadStreams = (state: GlobalState): UnreadStreamsState => state.unread.streams;
 
 export const getUnreadPms = (state: GlobalState): UnreadPmsState => state.unread.pms;
 
@@ -42,13 +37,150 @@ export const getUnreadHuddles = (state: GlobalState): UnreadHuddlesState => stat
 
 export const getUnreadMentions = (state: GlobalState): UnreadMentionsState => state.unread.mentions;
 
+//
+//
+// Reducer.
+//
+
+const initialStreamsState: UnreadStreamsState = Immutable.Map();
+
+// Like `Immutable.Map#map`, but with the update-only-if-different semantics
+// of `Immutable.Map#update`.  Kept for comparison to `updateAllAndPrune`.
+/* eslint-disable-next-line no-unused-vars */
+function updateAll<K, V>(map: Immutable.Map<K, V>, updater: V => V): Immutable.Map<K, V> {
+  return map.withMutations(mapMut => {
+    map.forEach((value, key) => {
+      const newValue = updater(value);
+      if (newValue !== value) {
+        mapMut.set(key, newValue);
+      }
+    });
+  });
+}
+
+// Like `updateAll`, but prune values equal to `zero` given by `updater`.
+function updateAllAndPrune<K, V>(
+  map: Immutable.Map<K, V>,
+  zero: V,
+  updater: V => V,
+): Immutable.Map<K, V> {
+  return map.withMutations(mapMut => {
+    map.forEach((value, key) => {
+      const newValue = updater(value);
+      if (newValue === zero) {
+        mapMut.delete(key);
+      }
+      if (newValue === value) {
+        return; // i.e., continue
+      }
+      mapMut.set(key, newValue);
+    });
+  });
+}
+
+function deleteMessages(
+  state: UnreadStreamsState,
+  ids: $ReadOnlyArray<number>,
+): UnreadStreamsState {
+  const idSet = new Set(ids);
+  const toDelete = id => idSet.has(id);
+  const emptyList = Immutable.List();
+  return updateAllAndPrune(state, Immutable.Map(), perStream =>
+    updateAllAndPrune(perStream, emptyList, perTopic =>
+      perTopic.find(toDelete) ? perTopic.filterNot(toDelete) : perTopic,
+    ),
+  );
+}
+
+function streamsReducer(
+  state: UnreadStreamsState = initialStreamsState,
+  action: Action,
+  globalState: GlobalState,
+): UnreadStreamsState {
+  switch (action.type) {
+    case LOGOUT:
+    case ACCOUNT_SWITCH:
+      // TODO(#4446) also LOGIN_SUCCESS, presumably
+      return initialStreamsState;
+
+    case REALM_INIT: {
+      // This may indeed be unnecessary, but it's legacy; have not investigated
+      // if it's this bit of our API types that is too optimistic.
+      // flowlint-next-line unnecessary-optional-chain:off
+      const data = action.data.unread_msgs?.streams ?? [];
+
+      const st = initialStreamsState.asMutable();
+      for (const { stream_id, topic, unread_message_ids } of data) {
+        // unread_message_ids is already sorted; see comment at its
+        // definition in src/api/initialDataTypes.js.
+        st.setIn([stream_id, topic], Immutable.List(unread_message_ids));
+      }
+      return st.asImmutable();
+    }
+
+    case MESSAGE_FETCH_COMPLETE:
+      // TODO handle MESSAGE_FETCH_COMPLETE here.  This rarely matters, but
+      //   could in principle: we could be fetching some messages from
+      //   before the (long) window included in the initial unreads data.
+      //   For comparison, the webapp does handle this case; see the call to
+      //   message_util.do_unread_count_updates in message_fetch.js.
+      return state;
+
+    case EVENT_NEW_MESSAGE: {
+      const { message } = action;
+      if (message.type !== 'stream') {
+        return state;
+      }
+
+      if (message.sender_id === getOwnUserId(globalState)) {
+        return state;
+      }
+
+      // prettier-ignore
+      return state.updateIn([message.stream_id, message.subject],
+        (perTopic = Immutable.List()) => perTopic.push(message.id));
+    }
+
+    case EVENT_MESSAGE_DELETE:
+      // TODO optimize by using `state.messages` to look up directly
+      return deleteMessages(state, action.messageIds);
+
+    case EVENT_UPDATE_MESSAGE_FLAGS: {
+      if (action.flag !== 'read') {
+        return state;
+      }
+
+      if (action.all) {
+        return initialStreamsState;
+      }
+
+      if (action.operation === 'remove') {
+        // Zulip doesn't support un-reading a message.  Ignore it.
+        return state;
+      }
+
+      // TODO optimize by using `state.messages` to look up directly.
+      //   Then when do, also optimize so deleting the oldest items is fast,
+      //   as that should be the common case here.
+      return deleteMessages(state, action.messages);
+    }
+
+    default:
+      return state;
+  }
+}
+
 export const reducer = (
   state: void | UnreadState,
   action: Action,
   globalState: GlobalState,
 ): UnreadState => {
   const nextState = {
-    streams: unreadStreamsReducer(state?.streams, action, globalState),
+    streams: streamsReducer(state?.streams, action, globalState),
+
+    // Note for converting these other sub-reducers to the new design:
+    // Probably first push this four-part data structure down through the
+    // `switch` statement, and the other logic that's duplicated between them.
     pms: unreadPmsReducer(state?.pms, action),
     huddles: unreadHuddlesReducer(state?.huddles, action),
     mentions: unreadMentionsReducer(state?.mentions, action),
diff --git a/src/unread/unreadModelTypes.js b/src/unread/unreadModelTypes.js
index e495bce9203..0af4f24ad32 100644
--- a/src/unread/unreadModelTypes.js
+++ b/src/unread/unreadModelTypes.js
@@ -1,9 +1,12 @@
 /* @flow strict-local */
+import Immutable from 'immutable';
 
-import type { HuddlesUnreadItem, PmsUnreadItem, StreamUnreadItem } from '../api/apiTypes';
+import type { HuddlesUnreadItem, PmsUnreadItem } from '../api/apiTypes';
 
 // These four are fragments of UnreadState; see below.
-export type UnreadStreamsState = StreamUnreadItem[];
+// prettier-ignore
+export type UnreadStreamsState =
+  Immutable.Map<number, Immutable.Map<string, Immutable.List<number>>>;
 export type UnreadHuddlesState = HuddlesUnreadItem[];
 export type UnreadPmsState = PmsUnreadItem[];
 export type UnreadMentionsState = number[];
diff --git a/src/unread/unreadStreamsReducer.js b/src/unread/unreadStreamsReducer.js
deleted file mode 100644
index e36d1bf0884..00000000000
--- a/src/unread/unreadStreamsReducer.js
+++ /dev/null
@@ -1,78 +0,0 @@
-/* @flow strict-local */
-import type { Action, GlobalState } from '../types';
-import type { UnreadStreamsState } from './unreadModelTypes';
-import {
-  REALM_INIT,
-  LOGOUT,
-  ACCOUNT_SWITCH,
-  EVENT_NEW_MESSAGE,
-  EVENT_MESSAGE_DELETE,
-  EVENT_UPDATE_MESSAGE_FLAGS,
-} from '../actionConstants';
-import { addItemsToStreamArray, removeItemsDeeply } from './unreadHelpers';
-import { NULL_ARRAY } from '../nullObjects';
-import { getOwnUserId } from '../users/userSelectors';
-
-const initialState: UnreadStreamsState = NULL_ARRAY;
-
-const eventNewMessage = (state, action, globalState) => {
-  if (action.message.type !== 'stream') {
-    return state;
-  }
-
-  if (getOwnUserId(globalState) === action.message.sender_id) {
-    return state;
-  }
-
-  return addItemsToStreamArray(
-    state,
-    [action.message.id],
-    action.message.stream_id,
-    action.message.subject,
-  );
-};
-
-const eventUpdateMessageFlags = (state, action) => {
-  if (action.flag !== 'read') {
-    return state;
-  }
-
-  if (action.all) {
-    return initialState;
-  }
-
-  if (action.operation === 'add') {
-    return removeItemsDeeply(state, action.messages);
-  } else if (action.operation === 'remove') {
-    // we do not support that operation
-  }
-
-  return state;
-};
-
-export default (
-  state: UnreadStreamsState = initialState,
-  action: Action,
-  globalState: GlobalState,
-): UnreadStreamsState => {
-  switch (action.type) {
-    case LOGOUT:
-    case ACCOUNT_SWITCH:
-      return initialState;
-
-    case REALM_INIT:
-      return (action.data.unread_msgs && action.data.unread_msgs.streams) || initialState;
-
-    case EVENT_NEW_MESSAGE:
-      return eventNewMessage(state, action, globalState);
-
-    case EVENT_MESSAGE_DELETE:
-      return removeItemsDeeply(state, action.messageIds);
-
-    case EVENT_UPDATE_MESSAGE_FLAGS:
-      return eventUpdateMessageFlags(state, action);
-
-    default:
-      return state;
-  }
-};

From 484929214f35f9c0ae2a559186f99a6f56130eb5 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Tue, 12 Jan 2021 18:02:19 -0800
Subject: [PATCH 11/14] unread tests: Add a test for not touching unaffected
 parts of state.

This is one property that only even becomes possible with the new
data structure!

This is useful in part because it might help downstream consumers
of the data make an optimization, by not recomputing their own work
if the particular piece of this data that they're interested in
hasn't changed.

The more immediate motivation for the test, though, is that it
effectively checks that we have a certain optimization working
within this model's own implementation.  A naive implementation
might end up copying the maps for all the streams when only one or
a few of them actually needed to change; in fact an early draft of
this implementation did just that.  If instead we successfully
returned the old maps, then that probably means we successfully
avoided doing the work of allocating new ones.
---
 src/unread/__tests__/unreadModel-test.js | 26 +++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/src/unread/__tests__/unreadModel-test.js b/src/unread/__tests__/unreadModel-test.js
index 2c47db90c45..4ae7ef4295f 100644
--- a/src/unread/__tests__/unreadModel-test.js
+++ b/src/unread/__tests__/unreadModel-test.js
@@ -146,8 +146,9 @@ describe('stream substate', () => {
       };
     };
 
+    const streamAction = args => mkMessageAction(eg.streamMessage(args));
+
     const baseState = (() => {
-      const streamAction = args => mkMessageAction(eg.streamMessage(args));
       const r = (state, action) => reducer(state, action, eg.plusReduxState);
       let state = initialState;
       state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 1 }));
@@ -184,6 +185,29 @@ describe('stream substate', () => {
       ]));
     });
 
+    test("when removing, don't touch unaffected topics or streams", () => {
+      const state = reducer(
+        baseState,
+        streamAction({ stream_id: 123, subject: 'qux', id: 7 }),
+        eg.plusReduxState,
+      );
+      // prettier-ignore
+      expect(summary(state)).toEqual(Immutable.Map([
+        [123, Immutable.Map([['foo', [1, 2, 3]], ['qux', [7]]])],
+        [234, Immutable.Map([['bar', [4, 5]]])],
+      ]));
+
+      const action = mkAction({ messages: [1, 2] });
+      const newState = reducer(state, action, eg.plusReduxState);
+      // prettier-ignore
+      expect(summary(newState)).toEqual(Immutable.Map([
+        [123, Immutable.Map([['foo', [3]], ['qux', [7]]])],
+        [234, Immutable.Map([['bar', [4, 5]]])],
+      ]));
+      expect(newState.streams.get(123)?.get('qux')).toBe(state.streams.get(123)?.get('qux'));
+      expect(newState.streams.get(234)).toBe(state.streams.get(234));
+    });
+
     test('when operation is "remove" do nothing', () => {
       const action = mkAction({ messages: [1, 2], operation: 'remove' });
       expect(reducer(baseState, action, eg.plusReduxState)).toBe(baseState);

From 8dc3fe605c5ee305555c515b2f938c83bd6d4ce8 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Wed, 27 Jan 2021 20:10:08 -0800
Subject: [PATCH 12/14] unread: Use state.messages to optimize updating stream
 unreads.

Before this commit, when a message was marked as read we'd have to
search through this entire data structure to find where it was so we
could remove it.  In fact, even if the message was actually a PM we'd
end up searching through this whole data structure which is entirely
about stream messages, because we didn't use that information.

The same thing was true with the old data structure, before this
series.

Much better would be, when a message in a particular conversation gets
marked as read, to go straight to that particular conversation's part
of the data structure and update that without having to search through
anything else.  Do that.

Knowing what conversation the message is in requires looking that
information up in our data structures.  Happily we can do that now
(and without an intrusive hack like we've sometimes done in the past):
that was #4437.

This reduces the time spent in this reducer to 7ms in the slowest
sample I've seen, or as little as <1ms (until recently the threshold
of measurement), and the total time spent in dispatch to 110-120ms.
Those compare with 30-50ms reducer / 150-200ms total before this
commit, and with 70ms reducer / 300ms total before the whole series,
using the old data structure.  (Based on measuring the same way as
described a few commits ago.)  So that's an improvement of about 2.5x,
or 180ms!

The 110-120ms we're still spending, almost all of it now outside the
reducer, still isn't *great*.  But it's enough better that I think
further optimization is no longer a top-priority thing for me to work
on; and because the remaining problem isn't inside the reducer where
I've been working and have built up the perf-logging tools to iterate
on, it's beyond the scope of where it's just easy to keep going.  So
with this I'm declaring victory on #4438, the task of making this
"unread" model efficient at marking a message as read.

Fixes: #4438
---
 src/unread/__tests__/unreadModel-test.js |  45 +++---
 src/unread/unreadModel.js                | 181 +++++++++++++++++------
 2 files changed, 167 insertions(+), 59 deletions(-)

diff --git a/src/unread/__tests__/unreadModel-test.js b/src/unread/__tests__/unreadModel-test.js
index 4ae7ef4295f..287c2d4c815 100644
--- a/src/unread/__tests__/unreadModel-test.js
+++ b/src/unread/__tests__/unreadModel-test.js
@@ -146,19 +146,28 @@ describe('stream substate', () => {
       };
     };
 
-    const streamAction = args => mkMessageAction(eg.streamMessage(args));
+    const messages = [
+      eg.streamMessage({ stream_id: 123, subject: 'foo', id: 1 }),
+      eg.streamMessage({ stream_id: 123, subject: 'foo', id: 2 }),
+      eg.streamMessage({ stream_id: 123, subject: 'foo', id: 3 }),
+      eg.streamMessage({ stream_id: 234, subject: 'bar', id: 4 }),
+      eg.streamMessage({ stream_id: 234, subject: 'bar', id: 5 }),
+    ];
 
     const baseState = (() => {
       const r = (state, action) => reducer(state, action, eg.plusReduxState);
       let state = initialState;
-      state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 1 }));
-      state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 2 }));
-      state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 3 }));
-      state = r(state, streamAction({ stream_id: 234, subject: 'bar', id: 4 }));
-      state = r(state, streamAction({ stream_id: 234, subject: 'bar', id: 5 }));
+      for (const message of messages) {
+        state = r(state, mkMessageAction(message));
+      }
       return state;
     })();
 
+    const baseGlobalState = eg.reduxStatePlus({
+      messages: eg.makeMessagesState(messages),
+      unread: baseState,
+    });
+
     test('(base state, for comparison)', () => {
       // prettier-ignore
       expect(summary(baseState)).toEqual(Immutable.Map([
@@ -169,28 +178,30 @@ describe('stream substate', () => {
 
     test('when operation is "add" but flag is not "read" do not mutate state', () => {
       const action = mkAction({ messages: [1, 2, 3], flag: 'star' });
-      expect(reducer(initialState, action, eg.plusReduxState)).toBe(initialState);
+      expect(reducer(initialState, action, baseGlobalState)).toBe(initialState);
     });
 
     test('if id does not exist do not mutate state', () => {
       const action = mkAction({ messages: [6, 7] });
-      expect(reducer(baseState, action, eg.plusReduxState)).toBe(baseState);
+      expect(reducer(baseState, action, baseGlobalState)).toBe(baseState);
     });
 
     test('if ids are in state remove them', () => {
       const action = mkAction({ messages: [3, 4, 5, 6] });
       // prettier-ignore
-      expect(summary(reducer(baseState, action, eg.plusReduxState))).toEqual(Immutable.Map([
+      expect(summary(reducer(baseState, action, baseGlobalState))).toEqual(Immutable.Map([
         [123, Immutable.Map([['foo', [1, 2]]])],
       ]));
     });
 
     test("when removing, don't touch unaffected topics or streams", () => {
-      const state = reducer(
-        baseState,
-        streamAction({ stream_id: 123, subject: 'qux', id: 7 }),
-        eg.plusReduxState,
-      );
+      const message = eg.streamMessage({ stream_id: 123, subject: 'qux', id: 7 });
+      const state = reducer(baseState, mkMessageAction(message), baseGlobalState);
+      const globalState = eg.reduxStatePlus({
+        messages: eg.makeMessagesState([...messages, message]),
+        unread: state,
+      });
+
       // prettier-ignore
       expect(summary(state)).toEqual(Immutable.Map([
         [123, Immutable.Map([['foo', [1, 2, 3]], ['qux', [7]]])],
@@ -198,7 +209,7 @@ describe('stream substate', () => {
       ]));
 
       const action = mkAction({ messages: [1, 2] });
-      const newState = reducer(state, action, eg.plusReduxState);
+      const newState = reducer(state, action, globalState);
       // prettier-ignore
       expect(summary(newState)).toEqual(Immutable.Map([
         [123, Immutable.Map([['foo', [3]], ['qux', [7]]])],
@@ -210,12 +221,12 @@ describe('stream substate', () => {
 
     test('when operation is "remove" do nothing', () => {
       const action = mkAction({ messages: [1, 2], operation: 'remove' });
-      expect(reducer(baseState, action, eg.plusReduxState)).toBe(baseState);
+      expect(reducer(baseState, action, baseGlobalState)).toBe(baseState);
     });
 
     test('when "all" is true reset state', () => {
       const action = mkAction({ messages: [], all: true });
-      expect(reducer(baseState, action, eg.plusReduxState).streams).toBe(initialState.streams);
+      expect(reducer(baseState, action, baseGlobalState).streams).toBe(initialState.streams);
     });
   });
 });
diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js
index f3090e6ef9c..624abe8347a 100644
--- a/src/unread/unreadModel.js
+++ b/src/unread/unreadModel.js
@@ -44,52 +44,153 @@ export const getUnreadMentions = (state: GlobalState): UnreadMentionsState => st
 
 const initialStreamsState: UnreadStreamsState = Immutable.Map();
 
-// Like `Immutable.Map#map`, but with the update-only-if-different semantics
-// of `Immutable.Map#update`.  Kept for comparison to `updateAllAndPrune`.
-/* eslint-disable-next-line no-unused-vars */
-function updateAll<K, V>(map: Immutable.Map<K, V>, updater: V => V): Immutable.Map<K, V> {
-  return map.withMutations(mapMut => {
-    map.forEach((value, key) => {
-      const newValue = updater(value);
-      if (newValue !== value) {
-        mapMut.set(key, newValue);
-      }
-    });
-  });
-}
-
-// Like `updateAll`, but prune values equal to `zero` given by `updater`.
-function updateAllAndPrune<K, V>(
+// Like `Immutable.Map#update`, but prune returned values equal to `zero`.
+function updateAndPrune<K, V>(
   map: Immutable.Map<K, V>,
   zero: V,
-  updater: V => V,
+  key: K,
+  updater: (V | void) => V,
 ): Immutable.Map<K, V> {
-  return map.withMutations(mapMut => {
-    map.forEach((value, key) => {
-      const newValue = updater(value);
-      if (newValue === zero) {
-        mapMut.delete(key);
-      }
-      if (newValue === value) {
-        return; // i.e., continue
-      }
-      mapMut.set(key, newValue);
-    });
-  });
+  const value = map.get(key);
+  const newValue = updater(value);
+  if (newValue === zero) {
+    return map.delete(key);
+  }
+  if (newValue === value) {
+    return map;
+  }
+  return map.set(key, newValue);
+}
+
+/**
+ * Remove the given values from the list.
+ *
+ * This is equivalent to
+ *   list_.filter(x => toDelete.indexOf(x) < 0)
+ * but more efficient.
+ *
+ * Specifically, for n items in the list and k to delete, this takes time
+ * O(n log n) in the worst case.
+ *
+ * In the case where the items to delete all appear at the beginning of the
+ * list, and in the same order, it takes time O(k log n).  (This is the
+ * common case when marking messages as read, which motivates this
+ * optimization.)
+ */
+// In principle this should be doable in time O(k + log n) in the
+// all-at-start case.  We'd need the iterator on Immutable.List to support
+// iterating through the first k elements in O(k + log n) time.  It seems
+// like it should be able to do that, but the current implementation (as of
+// Immutable 4.0.0-rc.12) takes time O(k log n): each step of the iterator
+// passes through a stack of log(n) helper functions.  Ah well.
+//
+// The logs are base 32, so in practice our log(n) is never more than 3
+// (which would be enough for 32**3 = 32768 items), usually at most 2
+// (enough for 1024 items); and for the messages in one conversation, very
+// commonly 1, i.e. there are commonly just ≤32 messages.  So the difference
+// between O(k log n) and O(k + log n) might be noticeable but is unlikely
+// to be catastrophic.
+function deleteFromList<V>(
+  list_: Immutable.List<V>,
+  toDelete_: Immutable.List<V>,
+): Immutable.List<V> {
+  // Alias the parameters because Flow doesn't accept mutating them.
+  let list = list_;
+  let toDelete = toDelete_;
+
+  // First, see if some items to delete happen to be at the start, and
+  // remove those.  This is the common case for marking messages as read,
+  // so it's worth some effort to optimize.  And we can do it efficiently:
+  // for deleting the first k out of n messages, we take time O(k log n)
+  // rather than O(n).
+
+  const minSize = Math.min(list.size, toDelete.size);
+  let i = 0;
+  for (; i < minSize; i++) {
+    // This loop takes time O(log n) per iteration, O(k log n) total.
+    if (list.get(i) !== toDelete.get(i)) {
+      break;
+    }
+  }
+
+  if (i > 0) {
+    // This takes time O(log n).
+    list = list.slice(i);
+    // This takes time O(log k) ≤ O(log n).
+    toDelete = toDelete.slice(i);
+  }
+
+  // That might have been all the items we wanted to delete.
+  // In fact that's the most common case when marking items as read.
+  if (toDelete.isEmpty()) {
+    return list;
+  }
+
+  // It wasn't; we have more to delete.  We'll have to find them in the
+  // middle of the list and delete them wherever they are.
+  //
+  // This takes time O(n log n), probably (though an ideal implementation of
+  // Immutable should be able to make it O(n).)
+  const toDeleteSet = new Set(toDelete);
+  return list.filterNot(id => toDeleteSet.has(id));
 }
 
+/**
+ * Delete the given messages from the unreads state.
+ *
+ * Relies on `globalMessages` to look up exactly where in the unreads data
+ * structure the messages are expected to appear.
+ *
+ * This is efficient at deleting some messages even when the total number of
+ * existing messages is much larger.  Specifically the time spent should be
+ * O(N' log n + c log C), where the messages to delete appear in c out of a
+ * total of C conversations, and the affected conversations have a total of
+ * N' messages and at most n in any one conversation.  If the messages to be
+ * deleted are all at the start of the list for their respective
+ * conversations the time should be O(k log n + c log C), where there are
+ * k messages to delete.
+ *
+ * For the common case of marking some messages as read, we expect that all
+ * the affected messages will indeed be at the start of their respective
+ * conversations, and the number c of affected conversations will be small,
+ * typically 1.  (It could be more than 1 if reading a stream narrow, or
+ * other interleaved narrow.)
+ */
 function deleteMessages(
   state: UnreadStreamsState,
   ids: $ReadOnlyArray<number>,
+  globalMessages,
 ): UnreadStreamsState {
-  const idSet = new Set(ids);
-  const toDelete = id => idSet.has(id);
+  const byConversation =
+    // prettier-ignore
+    (Immutable.Map(): Immutable.Map<number, Immutable.Map<string, Immutable.List<number>>>)
+    .withMutations(mut => {
+      for (const id of ids) {
+        const message = globalMessages.get(id);
+        if (!message || message.type !== 'stream') {
+          continue;
+        }
+        const { stream_id, subject: topic } = message;
+        mut.updateIn([stream_id, topic], (l = Immutable.List()) => l.push(id));
+      }
+    });
+
+  const emptyMap = Immutable.Map();
   const emptyList = Immutable.List();
-  return updateAllAndPrune(state, Immutable.Map(), perStream =>
-    updateAllAndPrune(perStream, emptyList, perTopic =>
-      perTopic.find(toDelete) ? perTopic.filterNot(toDelete) : perTopic,
-    ),
-  );
+  // prettier-ignore
+  return state.withMutations(stateMut => {
+    byConversation.forEach((byTopic, streamId) => {
+      updateAndPrune(stateMut, emptyMap, streamId, perStream =>
+        perStream && perStream.withMutations(perStreamMut => {
+          byTopic.forEach((msgIds, topic) => {
+            updateAndPrune(perStreamMut, emptyList, topic, perTopic =>
+              perTopic && deleteFromList(perTopic, msgIds),
+            );
+          });
+        }),
+      );
+    });
+  });
 }
 
 function streamsReducer(
@@ -142,8 +243,7 @@ function streamsReducer(
     }
 
     case EVENT_MESSAGE_DELETE:
-      // TODO optimize by using `state.messages` to look up directly
-      return deleteMessages(state, action.messageIds);
+      return deleteMessages(state, action.messageIds, globalState.messages);
 
     case EVENT_UPDATE_MESSAGE_FLAGS: {
       if (action.flag !== 'read') {
@@ -159,10 +259,7 @@ function streamsReducer(
         return state;
       }
 
-      // TODO optimize by using `state.messages` to look up directly.
-      //   Then when do, also optimize so deleting the oldest items is fast,
-      //   as that should be the common case here.
-      return deleteMessages(state, action.messages);
+      return deleteMessages(state, action.messages, globalState.messages);
     }
 
     default:

From 5725e7d8f375f2feba6e0116d1f5dc78afda4cf3 Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Thu, 28 Jan 2021 23:06:08 -0800
Subject: [PATCH 13/14] unread: Speed up initial-data loading by constructing
 in batch.

This substantially reduces the time we spend in constructing this
data structure at the initial fetch: from about 100ms, it goes down
to about 55ms.

The basic principle here is that if you have a whole bunch of items
you're going to build a data structure out of, it's faster to
construct the data structure with all of them at once than to start
with an empty data structure and add them one at a time.  That's
already what we routinely do in simple cases -- and in fact there's
a typical example at the innermost part of this code, the
`Immutable.List(unread_message_ids)`.

For this data structure as a whole (the Map of Maps of Lists), the
reason we didn't was just that the input comes to us as individual
topics, not grouped by stream.  But we can group them as a
preprocessing step, and then construct each Immutable data structure
all in one go without updates.  For the preprocessing step, we do need
a data structure we build up incrementally... but we can use plain
mutable built-in data structures for that, as it's temporary and
local; and even use just a plain Array for each stream's topics,
rather than any kind of Map, because we're only going to run through
the whole thing and never try to look up a particular item, or update
a particular existing item in it.  Those have a lot less overhead than
Immutable.Map, partly because they're built into the JS engine and
partly because an Array is a much simpler data structure than a map.
---
 src/unread/unreadModel.js | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js
index 624abe8347a..38a08f38fba 100644
--- a/src/unread/unreadModel.js
+++ b/src/unread/unreadModel.js
@@ -210,13 +210,26 @@ function streamsReducer(
       // flowlint-next-line unnecessary-optional-chain:off
       const data = action.data.unread_msgs?.streams ?? [];
 
-      const st = initialStreamsState.asMutable();
+      // First, collect together all the data for a given stream, just in a
+      // plain old Array.
+      const byStream = new Map();
       for (const { stream_id, topic, unread_message_ids } of data) {
+        let perStream = byStream.get(stream_id);
+        if (!perStream) {
+          perStream = [];
+          byStream.set(stream_id, perStream);
+        }
         // unread_message_ids is already sorted; see comment at its
         // definition in src/api/initialDataTypes.js.
-        st.setIn([stream_id, topic], Immutable.List(unread_message_ids));
+        perStream.push([topic, Immutable.List(unread_message_ids)]);
       }
-      return st.asImmutable();
+
+      // Then, for each of those plain Arrays build an Immutable.Map from it
+      // all in one shot.  This is quite a bit faster than building the Maps
+      // incrementally.  For a user with lots of unreads in a busy org, we
+      // can be handling 50k message IDs here, across perhaps 2-5k threads
+      // in dozens of streams, so the effect is significant.
+      return Immutable.Map(Immutable.Seq.Keyed(byStream.entries()).map(Immutable.Map));
     }
 
     case MESSAGE_FETCH_COMPLETE:

From 09cbcecd14a9110da7252b167ea4326f14c4c4bf Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Wed, 27 Jan 2021 21:41:59 -0800
Subject: [PATCH 14/14] unread [nfc]: Factor out deleteFromListMap.

Should be helpful when converting over the other parts of the
unread state.
---
 src/unread/unreadModel.js | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js
index 38a08f38fba..b81f048b5c6 100644
--- a/src/unread/unreadModel.js
+++ b/src/unread/unreadModel.js
@@ -135,6 +135,28 @@ function deleteFromList<V>(
   return list.filterNot(id => toDeleteSet.has(id));
 }
 
+const emptyList = Immutable.List();
+
+/**
+ * Remove the given values, given where to find them; and prune.
+ *
+ * That is, for each entry in `toDelete`, we apply `deleteFromList` with the
+ * given list to the corresponding entry in `state`.  When the resulting
+ * list is empty, we prune that entry entirely.
+ */
+function deleteFromListMap<K, V>(
+  state: Immutable.Map<K, Immutable.List<V>>,
+  toDelete: Immutable.Map<K, Immutable.List<V>>,
+): Immutable.Map<K, Immutable.List<V>> {
+  // prettier-ignore
+  return state.withMutations(mut => {
+    toDelete.forEach((msgIds, key) => {
+      updateAndPrune(mut, emptyList, key, list =>
+        list && deleteFromList(list, msgIds));
+    });
+  });
+}
+
 /**
  * Delete the given messages from the unreads state.
  *
@@ -176,18 +198,11 @@ function deleteMessages(
     });
 
   const emptyMap = Immutable.Map();
-  const emptyList = Immutable.List();
   // prettier-ignore
   return state.withMutations(stateMut => {
     byConversation.forEach((byTopic, streamId) => {
       updateAndPrune(stateMut, emptyMap, streamId, perStream =>
-        perStream && perStream.withMutations(perStreamMut => {
-          byTopic.forEach((msgIds, topic) => {
-            updateAndPrune(perStreamMut, emptyList, topic, perTopic =>
-              perTopic && deleteFromList(perTopic, msgIds),
-            );
-          });
-        }),
+        perStream && deleteFromListMap(perStream, byTopic),
       );
     });
   });