From 30bd1dcefd1a175c587bce5ba063869a0e696b07 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 31 May 2022 10:20:55 +0800 Subject: [PATCH] DEV: More efficiently trigger topic tracking state on state change (#16952) * When loading topics in bulk, only trigger state change callbacks after all the topics have been loaded and we determine that state has actually changed. * State change callbacks are also only triggered when state has changed. The use of JSON.stringify might raise some performance concerns here as this is a performance sensitive codepath. However, I measured the time for each `_setState` function call locally, by wrapping the function call with `performance.now()`, and did not see any significant overhead. --- .../app/models/topic-tracking-state.js | 67 +++++++++++++---- .../unit/models/topic-tracking-state-test.js | 71 ++++++++++++++++++- 2 files changed, 121 insertions(+), 17 deletions(-) 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 b6a7cd5c992..15a5a94241c 100644 --- a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js +++ b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js @@ -423,7 +423,17 @@ const TopicTrackingState = EmberObject.extend({ // make sure all the state is up to date with what is accurate // from the server - list.topics.forEach(this._syncStateFromListTopic); + const newStates = []; + + for (const topic of list.topics) { + const newState = this._newStateFromListTopic(topic); + + if (newState) { + newStates.push(newState); + } + } + + this.loadStates(newStates); // correct missing states, safeguard in case message bus is corrupt if (this._shouldCompensateState(list, filter, queryParams)) { @@ -610,14 +620,38 @@ const TopicTrackingState = EmberObject.extend({ }, loadStates(data) { - (data || []).forEach((topic) => { - this.modifyState(topic, topic); + if (!data || data.length === 0) { + return; + } + + const modified = data.every((topic) => { + return this._setState({ topic, data: topic, skipAfterStateChange: true }); }); + + if (modified) { + this._afterStateChange(); + } + }, + + _setState({ topic, data, skipAfterStateChange }) { + const stateKey = this._stateKey(topic); + const oldState = this.states.get(stateKey); + + if (!oldState || JSON.stringify(oldState) !== JSON.stringify(data)) { + this.states.set(stateKey, data); + + if (!skipAfterStateChange) { + this._afterStateChange(); + } + + return true; + } else { + return false; + } }, modifyState(topic, data) { - this.states.set(this._stateKey(topic), data); - this._afterStateChange(); + this._setState({ topic, data }); }, modifyStateProp(topic, prop, data) { @@ -661,16 +695,13 @@ const TopicTrackingState = EmberObject.extend({ // topic from the list (e.g. updates category, highest read post // number, tags etc.) @bind - _syncStateFromListTopic(topic) { + _newStateFromListTopic(topic) { const state = this.findState(topic.id) || {}; // make a new copy so we aren't modifying the state object directly while // we make changes const newState = { ...state }; - newState.topic_id = topic.id; - newState.notification_level = topic.notification_level; - // see ListableTopicSerializer for unread_posts/unseen and other // topic property logic if (topic.unseen) { @@ -687,7 +718,16 @@ const TopicTrackingState = EmberObject.extend({ return; } - newState.highest_post_number = topic.highest_post_number; + newState.topic_id = topic.id; + + if (topic.notification_level) { + newState.notification_level = topic.notification_level; + } + + if (topic.highest_post_number) { + newState.highest_post_number = topic.highest_post_number; + } + if (topic.category) { newState.category_id = topic.category.id; } @@ -696,7 +736,7 @@ const TopicTrackingState = EmberObject.extend({ newState.tags = topic.tags; } - this.modifyState(topic.id, newState); + return newState; }, // this stops sync of tracking state when list is filtered, in the past this @@ -791,12 +831,12 @@ const TopicTrackingState = EmberObject.extend({ } } - const old = this.findState(data); + const old = { ...this.findState(data) }; if (data.message_type === "latest") { this.notifyIncoming(data); - if ((old && old.tags) !== data.payload.tags) { + if (old.tags !== data.payload.tags) { this.modifyStateProp(data, "tags", data.payload.tags); this.incrementMessageCount(); } @@ -814,7 +854,6 @@ const TopicTrackingState = EmberObject.extend({ // we have, and then substitute inferred values for last_read_post_number // and notification_level. Any errors will be corrected when a // topic-list is loaded which includes the topic. - let payload = data.payload; if (old) { diff --git a/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js b/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js index 4e680091a4f..bea13230ff4 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js +++ b/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js @@ -23,6 +23,23 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { this.clock.restore(); }); + test("bulk loading states only calls onStateChange callback once", function (assert) { + const trackingState = TopicTrackingState.create(); + let stateCallbackCalled = 0; + + trackingState.onStateChange(() => { + stateCallbackCalled += 1; + }); + + trackingState.loadStates([ + { topic_id: 1 }, + { topic_id: 2 }, + { topic_id: 3 }, + ]); + + assert.strictEqual(stateCallbackCalled, 1, "callback is only called once"); + }); + test("tag counts", function (assert) { const trackingState = TopicTrackingState.create(); @@ -338,13 +355,54 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { ); }); + test("sync - no changes to state", function (assert) { + const trackingState = TopicTrackingState.create(); + + trackingState.loadStates([ + { topic_id: 111, last_read_post_number: null }, + { topic_id: 222, last_read_post_number: null }, + ]); + + let stateCallbackCalled = 0; + + trackingState.onStateChange(() => { + stateCallbackCalled += 1; + }); + + const list = { + topics: [ + Topic.create({ + id: 111, + last_read_post_number: null, + unseen: true, + }), + Topic.create({ + id: 222, + last_read_post_number: null, + unseen: true, + }), + ], + }; + + trackingState.sync(list, "unread"); + + assert.strictEqual(stateCallbackCalled, 0, "callback is not called"); + }); + test("sync - updates state to match list topic for unseen and unread/new topics", function (assert) { const trackingState = TopicTrackingState.create(); + trackingState.loadStates([ { topic_id: 111, last_read_post_number: 0 }, { topic_id: 222, last_read_post_number: 1 }, ]); + let stateCallbackCalled = 0; + + trackingState.onStateChange(() => { + stateCallbackCalled += 1; + }); + const list = { topics: [ Topic.create({ @@ -385,6 +443,8 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { 17, "last_read_post_number is highest_post_number - (unread + new)" ); + + assert.strictEqual(stateCallbackCalled, 1, "callback is only called once"); }); test("sync - states missing from the topic list are updated based on the selected filter", function (assert) { @@ -476,11 +536,14 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { }); test("state is modified and callback is called", function (assert) { - let stateCallbackCalled = false; + let stateCallbackCalled = 0; + trackingState.onStateChange(() => { - stateCallbackCalled = true; + stateCallbackCalled += 1; }); + publishToMessageBus(`/unread`, unreadTopicPayload); + assert.deepEqual( trackingState.findState(111), { @@ -496,9 +559,10 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { }, "topic state updated" ); + assert.strictEqual( stateCallbackCalled, - true, + 1, "state change callback called" ); }); @@ -597,6 +661,7 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { message_type: "dismiss_new", payload: { topic_ids: [112] }, }); + assert.strictEqual(trackingState.findState(112).is_seen, true); });