From 5558b7a3b2b8d8beec94cd901712e3f778cfa6e6 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 1 Nov 2018 14:43:15 -0600 Subject: [PATCH 1/6] Avoid hitting the SettingsStore thousands of times when generating room lists Should fix https://github.com/vector-im/riot-web/issues/7646 to some degree --- src/stores/RoomListStore.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 8909dbb489..527fc0fe2e 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -217,11 +217,16 @@ class RoomListStore extends Store { } }); + // Note: we check the settings up here instead of in the forEach or + // in the _recentsComparator to avoid hitting the SettingsStore a few + // thousand times. + const pinUnread = SettingsStore.getValue("pinUnreadRooms"); + const pinMentioned = SettingsStore.getValue("pinMentionedRooms"); Object.keys(lists).forEach((listKey) => { let comparator; switch (RoomListStore._listOrders[listKey]) { case "recent": - comparator = this._recentsComparator; + comparator = (roomA, roomB) => this._recentsComparator(roomA, roomB, pinUnread, pinMentioned); break; case "manual": default: @@ -262,10 +267,7 @@ class RoomListStore extends Store { } } - _recentsComparator(roomA, roomB) { - const pinUnread = SettingsStore.getValue("pinUnreadRooms"); - const pinMentioned = SettingsStore.getValue("pinMentionedRooms"); - + _recentsComparator(roomA, roomB, pinUnread, pinMentioned) { // We try and set the ordering to be Mentioned > Unread > Recent // assuming the user has the right settings, of course From 272acfa2f55b89f87658da91a503a2023ef83958 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 1 Nov 2018 14:46:39 -0600 Subject: [PATCH 2/6] Appease the linter --- src/stores/RoomListStore.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 527fc0fe2e..65148ec8c4 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -226,7 +226,9 @@ class RoomListStore extends Store { let comparator; switch (RoomListStore._listOrders[listKey]) { case "recent": - comparator = (roomA, roomB) => this._recentsComparator(roomA, roomB, pinUnread, pinMentioned); + comparator = (roomA, roomB) => { + return this._recentsComparator(roomA, roomB, pinUnread, pinMentioned); + }; break; case "manual": default: From 0c7aadb92b0d3b7042ff8658c33df5b0b697cefe Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 1 Nov 2018 16:28:13 -0600 Subject: [PATCH 3/6] Improve room list sort performance by caching common variables This won't help much if the user is in a ton of highly active rooms, but for the most part this will help those in thousands of rooms, many of which are likely to be quiet. Fixes https://github.com/vector-im/riot-web/issues/7646 Fixes https://github.com/vector-im/riot-web/issues/7645 (due to timestamp ordering) --- src/stores/RoomListStore.js | 135 ++++++++++++++++++++++++++++++++---- 1 file changed, 120 insertions(+), 15 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 65148ec8c4..e77debd2f2 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -54,6 +54,7 @@ class RoomListStore extends Store { "im.vector.fake.archived": [], }, ready: false, + roomCache: {}, // roomId => { cacheType => value } }; } @@ -85,6 +86,8 @@ class RoomListStore extends Store { !payload.isLiveUnfilteredRoomTimelineEvent || !this._eventTriggersRecentReorder(payload.event) ) break; + + this._clearCachedRoomState(payload.event.getRoomId()); this._generateRoomLists(); } break; @@ -112,6 +115,8 @@ class RoomListStore extends Store { if (liveTimeline !== eventTimeline || !this._eventTriggersRecentReorder(payload.event) ) break; + + this._clearCachedRoomState(payload.event.getRoomId()); this._generateRoomLists(); } break; @@ -222,12 +227,20 @@ class RoomListStore extends Store { // thousand times. const pinUnread = SettingsStore.getValue("pinUnreadRooms"); const pinMentioned = SettingsStore.getValue("pinMentionedRooms"); + this._timings = {}; Object.keys(lists).forEach((listKey) => { let comparator; switch (RoomListStore._listOrders[listKey]) { case "recent": comparator = (roomA, roomB) => { - return this._recentsComparator(roomA, roomB, pinUnread, pinMentioned); + this._timings["overall_" + roomA.roomId + "_" + roomB.roomId] = { + type: "overall", + start: performance.now(), + end: 0, + }; + const ret = this._recentsComparator(roomA, roomB, pinUnread, pinMentioned); + this._timings["overall_" + roomA.roomId + "_" + roomB.roomId].end = performance.now(); + return ret; }; break; case "manual": @@ -238,12 +251,76 @@ class RoomListStore extends Store { lists[listKey].sort(comparator); }); + // Combine the samples for performance metrics + const samplesByType = {}; + for (const sampleName of Object.keys(this._timings)) { + const sample = this._timings[sampleName]; + if (!samplesByType[sample.type]) samplesByType[sample.type] = { + min: 999999999, + max: 0, + count: 0, + total: 0, + }; + + const record = samplesByType[sample.type]; + const duration = sample.end - sample.start; + if (duration < record.min) record.min = duration; + if (duration > record.max) record.max = duration; + record.count++; + record.total += duration; + } + + for (const category of Object.keys(samplesByType)) { + const {min, max, count, total} = samplesByType[category]; + const average = total / count; + + console.log(`RoomListSortPerf : type=${category} min=${min} max=${max} total=${total} samples=${count} average=${average}`); + } + this._setState({ lists, ready: true, // Ready to receive updates via Room.tags events }); } + _updateCachedRoomState(roomId, type, value) { + const roomCache = this._state.roomCache; + if (!roomCache[roomId]) roomCache[roomId] = {}; + + if (value) roomCache[roomId][type] = value; + else delete roomCache[roomId][type]; + + this._setState({roomCache}); + } + + _clearCachedRoomState(roomId) { + const roomCache = this._state.roomCache; + delete roomCache[roomId]; + this._setState({roomCache}); + } + + _getRoomState(room, type) { + const roomId = room.roomId; + const roomCache = this._state.roomCache; + if (roomCache[roomId] && typeof roomCache[roomId][type] !== 'undefined') { + return roomCache[roomId][type]; + } + + if (type === "timestamp") { + const ts = this._tsOfNewestEvent(room); + this._updateCachedRoomState(roomId, "timestamp", ts); + return ts; + } else if (type === "unread") { + const unread = room.getUnreadNotificationCount() > 0; + this._updateCachedRoomState(roomId, "unread", unread); + return unread; + } else if (type === "notifications") { + const notifs = room.getUnreadNotificationCount("highlight") > 0; + this._updateCachedRoomState(roomId, "notifications", notifs); + return notifs; + } else throw new Error("Unrecognized room cache type: " + type); + } + _eventTriggersRecentReorder(ev) { return ev.getTs() && ( Unread.eventTriggersUnreadCount(ev) || @@ -270,30 +347,58 @@ class RoomListStore extends Store { } _recentsComparator(roomA, roomB, pinUnread, pinMentioned) { + //console.log("Comparing " + roomA.roomId + " with " + roomB.roomId +" || pinUnread=" + pinUnread +" pinMentioned="+pinMentioned); // We try and set the ordering to be Mentioned > Unread > Recent - // assuming the user has the right settings, of course + // assuming the user has the right settings, of course. + + this._timings["timestamp_" + roomA.roomId + "_" + roomB.roomId] = { + type: "timestamp", + start: performance.now(), + end: 0, + }; + const timestampA = this._getRoomState(roomA, "timestamp"); + const timestampB = this._getRoomState(roomB, "timestamp"); + const timestampDiff = timestampB - timestampA; + this._timings["timestamp_" + roomA.roomId + "_" + roomB.roomId].end = performance.now(); if (pinMentioned) { - const mentionsA = roomA.getUnreadNotificationCount("highlight") > 0; - const mentionsB = roomB.getUnreadNotificationCount("highlight") > 0; - if (mentionsA && !mentionsB) return -1; - if (!mentionsA && mentionsB) return 1; - if (mentionsA && mentionsB) return 0; - // If neither have mentions, fall through to remaining checks + this._timings["mentioned_" + roomA.roomId + "_" + roomB.roomId] = { + type: "mentioned", + start: performance.now(), + end: 0, + }; + const mentionsA = this._getRoomState(roomA, "notifications"); + const mentionsB = this._getRoomState(roomB, "notifications"); + this._timings["mentioned_" + roomA.roomId + "_" + roomB.roomId].end = performance.now(); + if (mentionsA && !mentionsB) return -1; + if (!mentionsA && mentionsB) return 1; + + // If they both have notifications, sort by timestamp. + // If neither have notifications (the fourth check not shown + // here), then try and sort by unread messages and finally by + // timestamp. + if (mentionsA && mentionsB) return timestampDiff; } if (pinUnread) { - const unreadA = Unread.doesRoomHaveUnreadMessages(roomA); - const unreadB = Unread.doesRoomHaveUnreadMessages(roomB); + this._timings["unread_" + roomA.roomId + "_" + roomB.roomId] = { + type: "unread", + start: performance.now(), + end: 0, + }; + const unreadA = this._getRoomState(roomA, "unread"); + const unreadB = this._getRoomState(roomB, "notifications"); + this._timings["unread_" + roomA.roomId + "_" + roomB.roomId].end = performance.now(); if (unreadA && !unreadB) return -1; if (!unreadA && unreadB) return 1; - if (unreadA && unreadB) return 0; - // If neither have unread messages, fall through to remaining checks + + // If they both have unread messages, sort by timestamp + // If nether have unread message (the fourth check not shown + // here), then just sort by timestamp anyways. + if (unreadA && unreadB) return timestampDiff; } - // XXX: We could use a cache here and update it when we see new - // events that trigger a reorder - return this._tsOfNewestEvent(roomB) - this._tsOfNewestEvent(roomA); + return timestampDiff; } _lexicographicalComparator(roomA, roomB) { From 122868e32f3c27b1565c1044b3c325b96c5a0b54 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 1 Nov 2018 16:30:48 -0600 Subject: [PATCH 4/6] Removing timing/performance tracking on room list store This was used to verify the fix was actually making improvements and can be safely taken out. --- src/stores/RoomListStore.js | 55 +------------------------------------ 1 file changed, 1 insertion(+), 54 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index e77debd2f2..8dc557ace0 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -227,20 +227,12 @@ class RoomListStore extends Store { // thousand times. const pinUnread = SettingsStore.getValue("pinUnreadRooms"); const pinMentioned = SettingsStore.getValue("pinMentionedRooms"); - this._timings = {}; Object.keys(lists).forEach((listKey) => { let comparator; switch (RoomListStore._listOrders[listKey]) { case "recent": comparator = (roomA, roomB) => { - this._timings["overall_" + roomA.roomId + "_" + roomB.roomId] = { - type: "overall", - start: performance.now(), - end: 0, - }; - const ret = this._recentsComparator(roomA, roomB, pinUnread, pinMentioned); - this._timings["overall_" + roomA.roomId + "_" + roomB.roomId].end = performance.now(); - return ret; + return this._recentsComparator(roomA, roomB, pinUnread, pinMentioned); }; break; case "manual": @@ -251,32 +243,6 @@ class RoomListStore extends Store { lists[listKey].sort(comparator); }); - // Combine the samples for performance metrics - const samplesByType = {}; - for (const sampleName of Object.keys(this._timings)) { - const sample = this._timings[sampleName]; - if (!samplesByType[sample.type]) samplesByType[sample.type] = { - min: 999999999, - max: 0, - count: 0, - total: 0, - }; - - const record = samplesByType[sample.type]; - const duration = sample.end - sample.start; - if (duration < record.min) record.min = duration; - if (duration > record.max) record.max = duration; - record.count++; - record.total += duration; - } - - for (const category of Object.keys(samplesByType)) { - const {min, max, count, total} = samplesByType[category]; - const average = total / count; - - console.log(`RoomListSortPerf : type=${category} min=${min} max=${max} total=${total} samples=${count} average=${average}`); - } - this._setState({ lists, ready: true, // Ready to receive updates via Room.tags events @@ -347,29 +313,16 @@ class RoomListStore extends Store { } _recentsComparator(roomA, roomB, pinUnread, pinMentioned) { - //console.log("Comparing " + roomA.roomId + " with " + roomB.roomId +" || pinUnread=" + pinUnread +" pinMentioned="+pinMentioned); // We try and set the ordering to be Mentioned > Unread > Recent // assuming the user has the right settings, of course. - this._timings["timestamp_" + roomA.roomId + "_" + roomB.roomId] = { - type: "timestamp", - start: performance.now(), - end: 0, - }; const timestampA = this._getRoomState(roomA, "timestamp"); const timestampB = this._getRoomState(roomB, "timestamp"); const timestampDiff = timestampB - timestampA; - this._timings["timestamp_" + roomA.roomId + "_" + roomB.roomId].end = performance.now(); if (pinMentioned) { - this._timings["mentioned_" + roomA.roomId + "_" + roomB.roomId] = { - type: "mentioned", - start: performance.now(), - end: 0, - }; const mentionsA = this._getRoomState(roomA, "notifications"); const mentionsB = this._getRoomState(roomB, "notifications"); - this._timings["mentioned_" + roomA.roomId + "_" + roomB.roomId].end = performance.now(); if (mentionsA && !mentionsB) return -1; if (!mentionsA && mentionsB) return 1; @@ -381,14 +334,8 @@ class RoomListStore extends Store { } if (pinUnread) { - this._timings["unread_" + roomA.roomId + "_" + roomB.roomId] = { - type: "unread", - start: performance.now(), - end: 0, - }; const unreadA = this._getRoomState(roomA, "unread"); const unreadB = this._getRoomState(roomB, "notifications"); - this._timings["unread_" + roomA.roomId + "_" + roomB.roomId].end = performance.now(); if (unreadA && !unreadB) return -1; if (!unreadA && unreadB) return 1; From a713cc5c52309f74d1dfccef28556de037eb3db1 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 1 Nov 2018 17:07:05 -0600 Subject: [PATCH 5/6] Compare the right types of events --- src/stores/RoomListStore.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index 8dc557ace0..f7596774b6 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -335,7 +335,7 @@ class RoomListStore extends Store { if (pinUnread) { const unreadA = this._getRoomState(roomA, "unread"); - const unreadB = this._getRoomState(roomB, "notifications"); + const unreadB = this._getRoomState(roomB, "unread"); if (unreadA && !unreadB) return -1; if (!unreadA && unreadB) return 1; From 3960ae2fcd956702c5adc4f0929438acdea53388 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 1 Nov 2018 17:17:01 -0600 Subject: [PATCH 6/6] Add more commentary around how the roomCache works --- src/stores/RoomListStore.js | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index f7596774b6..2b70d53b59 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -54,7 +54,24 @@ class RoomListStore extends Store { "im.vector.fake.archived": [], }, ready: false, - roomCache: {}, // roomId => { cacheType => value } + + // The room cache stores a mapping of roomId to cache record. + // Each cache record is a key/value pair for various bits of + // data used to sort the room list. Currently this stores the + // following bits of informations: + // "timestamp": number, The timestamp of the last relevant + // event in the room. + // "notifications": boolean, Whether or not the user has been + // highlighted on any unread events. + // "unread": boolean, Whether or not the user has any + // unread events. + // + // All of the cached values are lazily loaded on read in the + // recents comparator. When an event is received for a particular + // room, all the cached values are invalidated - forcing the + // next read to set new values. The entries do not expire on + // their own. + roomCache: {}, }; }