DEV: Topic tracking state improvements (#12958)

DEV: Topic tracking state improvements (#12958)

Original PR which had to be reverted DEV: Improving topic tracking state code by martin-brennan · Pull Request #12555 · discourse/discourse · GitHub. See the description there for what this PR is achieving, plus below.

The issue with the original PR is addressed in DEV: Topic tracking state improvements by martin-brennan · Pull Request #12958 · discourse/discourse · GitHub

If you went to the x unread link for a tag Chrome would freeze up and possibly crash, or eventually unfreeze after nearly 10 mins. Other routes for unread/new were similarly slow. From profiling the issue was the sync function of topic-tracking-state.js, which calls down to isNew which in turn calls moment, a change I had made in the PR above. The time it takes locally with ~1400 topics in the tracking state is 2.3 seconds.

To solve this issue, I have moved these calculations for “created in new period” and “unread not too old” into the tracking state serializer.

When I was looking at the profiler I also noticed this issue which was just compounding the problem. Every time we modify topic tracking state we recalculate the sidebar tracking/everything/tag counts. However this calls forEachTracked and countTags which can be quite expensive as they go through the whole tracking state (and were also calling the removed moment functions).

I added some logs and this was being called 30 times when navigating to a new /unread route because sync is being called from build-topic-route (one for each topic loaded due to pagination). So I just added a debounce here and it makes things even faster.

Finally, I changed topic tracking state to use a Map so our counts of the state keys is faster (Maps have .size whereas objects you have to do Object.keys(obj) which is O(n).)

diff --git a/app/assets/javascripts/discourse/app/components/discovery-topics-list.js b/app/assets/javascripts/discourse/app/components/discovery-topics-list.js
index 8665981..9b414b9 100644
--- a/app/assets/javascripts/discourse/app/components/discovery-topics-list.js
+++ b/app/assets/javascripts/discourse/app/components/discovery-topics-list.js
@@ -21,8 +21,19 @@ const DiscoveryTopicsListComponent = Component.extend(UrlRefresh, LoadMore, {
     }
   },
 
-  @observes("topicTrackingState.states")
-  _updateTopics() {
+  @on("didInsertElement")
+  _monitorTrackingState() {
+    this.stateChangeCallbackId = this.topicTrackingState.onStateChange(
+      this._updateTrackingTopics.bind(this)
+    );
+  },
+
+  @on("willDestroyElement")
+  _removeTrackingStateChangeMonitor() {
+    this.topicTrackingState.offStateChange(this.stateChangeCallbackId);
+  },
+
+  _updateTrackingTopics() {
     this.topicTrackingState.updateTopics(this.model.topics);
   },
 
diff --git a/app/assets/javascripts/discourse/app/mixins/bulk-topic-selection.js b/app/assets/javascripts/discourse/app/mixins/bulk-topic-selection.js
index f2a1177..edcae9b 100644
--- a/app/assets/javascripts/discourse/app/mixins/bulk-topic-selection.js
+++ b/app/assets/javascripts/discourse/app/mixins/bulk-topic-selection.js
@@ -51,9 +51,7 @@ export default Mixin.create({
 
       promise.then((result) => {
         if (result && result.topic_ids) {
-          const tracker = this.topicTrackingState;
-          result.topic_ids.forEach((t) => tracker.removeTopic(t));
-          tracker.incrementMessageCount();
+          this.topicTrackingState.removeTopics(result.topic_ids);
         }
 
         this.send("closeModal");
diff --git a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js
index b00e842..8b424ec 100644
--- a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js
+++ b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js
@@ -1,11 +1,11 @@
 import EmberObject, { get } from "@ember/object";
 import discourseComputed, { on } from "discourse-common/utils/decorators";
 import Category from "discourse/models/category";
+import { deepEqual, deepMerge } from "discourse-common/lib/object";
 import DiscourseURL from "discourse/lib/url";
 import { NotificationLevels } from "discourse/lib/notification-levels";
 import PreloadStore from "discourse/lib/preload-store";
 import User from "discourse/models/user";
-import { deepEqual } from "discourse-common/lib/object";
 import { isEmpty } from "@ember/utils";
 
 function isNew(topic) {
@@ -13,6 +13,7 @@ function isNew(topic) {
     topic.last_read_post_number === null &&
     ((topic.notification_level !== 0 && !topic.notification_level) ||
       topic.notification_level >= NotificationLevels.TRACKING) &&
+    topic.created_in_new_period &&
     isUnseen(topic)
   );
 }
@@ -21,7 +22,8 @@ function isUnread(topic) {
   return (
     topic.last_read_post_number !== null &&
     topic.last_read_post_number < topic.highest_post_number &&
-    topic.notification_level >= NotificationLevels.TRACKING
+    topic.notification_level >= NotificationLevels.TRACKING &&
+    topic.unread_not_too_old
   );
 }
 
@@ -48,105 +50,47 @@ const TopicTrackingState = EmberObject.extend({
   _setup() {
     this.unreadSequence = [];
     this.newSequence = [];
-    this.states = {};
+    this.states = new Map();
+    this.messageIncrementCallbacks = {};
+    this.stateChangeCallbacks = {};
+    this._trackedTopicLimit = 4000;
   },
 
+  /**
+   * Subscribe to MessageBus channels which are used for publishing changes
+   * to the tracking state. Each message received will modify state for
+   * a particular topic.
+   *
+   * See app/models/topic_tracking_state.rb for the data payloads published
+   * to each of the channels.
+   *
+   * @method establishChannels
+   */
   establishChannels() {
-    const tracker = this;
-
-    const process = (data) => {
-      if (["muted", "unmuted"].includes(data.message_type)) {
-        tracker.trackMutedOrUnmutedTopic(data);
-        return;
-      }
-
-      tracker.pruneOldMutedAndUnmutedTopics();
-
-      if (tracker.isMutedTopic(data.topic_id)) {
-        return;
-      }
-
-      if (
-        this.siteSettings.mute_all_categories_by_default &&
-        !tracker.isUnmutedTopic(data.topic_id)
-      ) {
-        return;
-      }
-
-      if (data.message_type === "delete") {
-        tracker.removeTopic(data.topic_id);
-        tracker.incrementMessageCount();
-      }
-
-      if (["new_topic", "latest"].includes(data.message_type)) {
-        const muted_category_ids = User.currentProp("muted_category_ids");
-        if (
-          muted_category_ids &&
-          muted_category_ids.includes(data.payload.category_id)
-        ) {
-          return;
-        }
-      }
-
-      if (["new_topic", "latest"].includes(data.message_type)) {
-        const mutedTagIds = User.currentProp("muted_tag_ids");
-        if (
-          hasMutedTags(
-            data.payload.topic_tag_ids,
-            mutedTagIds,
-            this.siteSettings
-          )
-        ) {
-          return;
-        }
-      }
-
-      if (data.message_type === "latest") {
-        tracker.notify(data);
-      }
-
-      if (data.message_type === "dismiss_new") {
-        tracker.dismissNewTopic(data);
-      }
-
-      if (["new_topic", "unread", "read"].includes(data.message_type)) {
-        tracker.notify(data);
-        const old = tracker.states["t" + data.topic_id];
-        if (!deepEqual(old, data.payload)) {
-          tracker.states["t" + data.topic_id] = data.payload;
-          tracker.notifyPropertyChange("states");
-          tracker.incrementMessageCount();
-        }
-      }
-    };
-
-    this.messageBus.subscribe("/new", process);
-    this.messageBus.subscribe("/latest", process);
+    this.messageBus.subscribe("/new", this._processChannelPayload.bind(this));
+    this.messageBus.subscribe(
+      "/latest",
+      this._processChannelPayload.bind(this)
+    );
     if (this.currentUser) {
       this.messageBus.subscribe(
         "/unread/" + this.currentUser.get("id"),
-        process
+        this._processChannelPayload.bind(this)
       );
     }
 
     this.messageBus.subscribe("/delete", (msg) => {
-      const old = tracker.states["t" + msg.topic_id];
-      if (old) {
-        old.deleted = true;
-      }
-      tracker.incrementMessageCount();
+      this.modifyStateProp(msg, "deleted", true);
+      this.incrementMessageCount();
     });
 
     this.messageBus.subscribe("/recover", (msg) => {
-      const old = tracker.states["t" + msg.topic_id];
-      if (old) {
-        delete old.deleted;
-      }
-      tracker.incrementMessageCount();
+      this.modifyStateProp(msg, "deleted", false);
+      this.incrementMessageCount();
     });
 
     this.messageBus.subscribe("/destroy", (msg) => {
-      tracker.incrementMessageCount();
+      this.incrementMessageCount();
       const currentRoute = DiscourseURL.router.currentRoute.parent;
       if (
         currentRoute.name === "topic" &&
@@ -181,17 +125,6 @@ const TopicTrackingState = EmberObject.extend({
     this.currentUser && this.currentUser.set(key, topics);
   },
 
-  dismissNewTopic(data) {
-    data.payload.topic_ids.forEach((k) => {
-      const topic = this.states[`t${k}`];
-      this.states[`t${k}`] = Object.assign({}, topic, {
-        is_seen: true,
-      });
-    });
-    this.notifyPropertyChange("states");
-    this.incrementMessageCount();
-  },
-
   pruneOldMutedAndUnmutedTopics() {
     const now = Date.now();
     let mutedTopics = this.mutedTopics().filter(
@@ -213,22 +146,50 @@ const TopicTrackingState = EmberObject.extend({
     return !!this.unmutedTopics().findBy("topicId", topicId);
   },
 
+  /**
+   * Updates the topic's last_read_post_number to the highestSeen post
+   * number, as long as the topic is being tracked.
+   *

[... diff too long, it was truncated ...]

GitHub sha: 002c6763

This commit appears in #12958 which was approved by lis2. It was merged by martin.