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 zulip#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
existing tools), and the longest I've seen is 3ms.  The total dispatch
time varies widely, like 70-200ms, and it's 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: zulip#4438

Author: gnprice

src/boot/store.js

@@ -253,6 +253,9 @@ const migrations: { [string]: (GlobalState) => GlobalState } = {
   // Convert `messages` from object-as-map to `Immutable.Map`.
   '23': dropCache,
 
+  // Convert `unread.streams` from over-the-wire array to `Immutable.Map`.
+  '24': dropCache,
+
   // TIP: When adding a migration, consider just using `dropCache`.
 };
 

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', () => {

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,
-    },
-  ];
-};

src/unread/unreadModel.js

@@ -1,54 +1,187 @@
 /* @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;
 
 export const getUnreadHuddles = (state: GlobalState): UnreadHuddlesState => state.unread.huddles;
 
 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 === value) {
+        return; // i.e., continue
+      }
+      if (newValue === zero) {
+        mapMut.delete(key);
+      } else {
+        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),

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[];