PERF: Avoid triggering TopicTrackingState change callbacks unnecessarily (#21425)

What is the problem?

The TopicTrackingState is a service on the client side that is used to store
state of topics which is new or has unread posts for a given user. The state
is updated via various means and the one in concern here is whenever we load
a new topic list from the server. When a topic list is loaded from the server,
we sync this new topic list with the states in TopicTrackingState. There is also
a hard limit on the number of states that is stored by TopicTrackingState for
performance reasons and the limit is currently set to 4000. It was noticed that
once this limit has been reached, syncing a topic list with TopicTrackingState can
result in the registered state change callbacks to be called unnecessarily. This
is because during `TopicTrackingState#sync` we call `TopicTrackingState#removeTopic`
if the topic in question is neither new or unread to a user. However, `TopicTrackingState#removeTopic`
would call `TopicTrackingState#_afterStateChange` even if nothing was removed.

What is the fix?

This commit fixes the problem by checking that `TopicTrackingState#_afterStateChange` is only
called in `TopicTrackingState#removeTopic` when a topic is actually removed.
This commit is contained in:
Alan Guo Xiang Tan 2023-05-08 11:26:28 +08:00 committed by GitHub
parent ac0673d29e
commit 18c542dc02
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 30 additions and 2 deletions

View File

@ -397,8 +397,9 @@ const TopicTrackingState = EmberObject.extend({
* @method removeTopic * @method removeTopic
*/ */
removeTopic(topicId) { removeTopic(topicId) {
this.states.delete(this._stateKey(topicId)); if (this.states.delete(this._stateKey(topicId))) {
this._afterStateChange(); this._afterStateChange();
}
}, },
/** /**

View File

@ -335,6 +335,12 @@ module("Unit | Model | topic-tracking-state", function (hooks) {
trackingState.loadStates([{ topic_id: 111 }, { topic_id: 222 }]); trackingState.loadStates([{ topic_id: 111 }, { topic_id: 222 }]);
trackingState.set("_trackedTopicLimit", 1); trackingState.set("_trackedTopicLimit", 1);
let stateChangeCallbackCalledTimes = 0;
trackingState.onStateChange(() => {
stateChangeCallbackCalledTimes += 1;
});
const list = { const list = {
topics: [ topics: [
this.store.createRecord("topic", { this.store.createRecord("topic", {
@ -344,15 +350,36 @@ module("Unit | Model | topic-tracking-state", function (hooks) {
unread_posts: 0, unread_posts: 0,
prevent_sync: false, prevent_sync: false,
}), }),
this.store.createRecord("topic", {
id: 333,
unseen: false,
seen: true,
unread_posts: 0,
prevent_sync: false,
}),
this.store.createRecord("topic", {
id: 444,
unseen: false,
seen: true,
unread_posts: 0,
prevent_sync: false,
}),
], ],
}; };
trackingState.sync(list, "unread"); trackingState.sync(list, "unread");
assert.notOk( assert.notOk(
trackingState.states.has("t111"), trackingState.states.has("t111"),
"expect state for topic 111 to be deleted" "expect state for topic 111 to be deleted"
); );
assert.equal(
stateChangeCallbackCalledTimes,
1,
"callback is only called once"
);
trackingState.loadStates([{ topic_id: 111 }, { topic_id: 222 }]); trackingState.loadStates([{ topic_id: 111 }, { topic_id: 222 }]);
trackingState.set("_trackedTopicLimit", 5); trackingState.set("_trackedTopicLimit", 5);
trackingState.sync(list, "unread"); trackingState.sync(list, "unread");