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

Author: gnprice

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,36 +178,38 @@ 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]]])],
         [234, Immutable.Map([['bar', [4, 5]]])],
       ]));
 
       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);
     });
   });
 });

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: