From 1315f34662426b0b6be9ddf5002d0719224903d5 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 9 Jul 2020 14:52:54 -0600 Subject: [PATCH 1/5] Dedupe room list store updates by marking for updates The core of this is in the MarkedExecution class, with the remainder of the commit ensuring that the right marks and triggers are in place to do the firing. Because everything is async/await and run through the RoomListStore, we don't have to worry about self-fed updates in the algorithm classes. This also means we have to trigger pretty much all the time. Changes to tag ordering / list sorting get hit through two paths, so we mark before we do a bulk update and otherwise assume the call is coming in from outside. --- src/stores/room-list/RoomListStore2.ts | 54 +++++++++++----- src/stores/room-list/algorithms/Algorithm.ts | 10 +-- src/utils/MarkedExecution.ts | 67 ++++++++++++++++++++ 3 files changed, 108 insertions(+), 23 deletions(-) create mode 100644 src/utils/MarkedExecution.ts diff --git a/src/stores/room-list/RoomListStore2.ts b/src/stores/room-list/RoomListStore2.ts index 6020e46a12..c29c0b5a20 100644 --- a/src/stores/room-list/RoomListStore2.ts +++ b/src/stores/room-list/RoomListStore2.ts @@ -33,6 +33,7 @@ import { EffectiveMembership, getEffectiveMembership } from "./membership"; import { ListLayout } from "./ListLayout"; import { isNullOrUndefined } from "matrix-js-sdk/src/utils"; import RoomListLayoutStore from "./RoomListLayoutStore"; +import { MarkedExecution } from "../../utils/MarkedExecution"; interface IState { tagsEnabled?: boolean; @@ -51,7 +52,7 @@ export class RoomListStore2 extends AsyncStore { private algorithm = new Algorithm(); private filterConditions: IFilterCondition[] = []; private tagWatcher = new TagWatcher(this); - private layoutMap: Map = new Map(); + private updateFn = new MarkedExecution(() => this.emit(LISTS_UPDATE_EVENT)); private readonly watchedSettings = [ 'feature_custom_tags', @@ -91,24 +92,26 @@ export class RoomListStore2 extends AsyncStore { await this.updateAlgorithmInstances(); } - private onRVSUpdate = () => { + private onRVSUpdate = async (quiet = false) => { if (!this.enabled) return; // TODO: Remove with https://github.com/vector-im/riot-web/issues/14231 if (!this.matrixClient) return; // We assume there won't be RVS updates without a client const activeRoomId = RoomViewStore.getRoomId(); if (!activeRoomId && this.algorithm.stickyRoom) { - this.algorithm.stickyRoom = null; + await this.algorithm.setStickyRoom(null); } else if (activeRoomId) { const activeRoom = this.matrixClient.getRoom(activeRoomId); if (!activeRoom) { console.warn(`${activeRoomId} is current in RVS but missing from client - clearing sticky room`); - this.algorithm.stickyRoom = null; + await this.algorithm.setStickyRoom(null); } else if (activeRoom !== this.algorithm.stickyRoom) { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`Changing sticky room to ${activeRoomId}`); - this.algorithm.stickyRoom = activeRoom; + await this.algorithm.setStickyRoom(activeRoom); } } + + if (!quiet) this.updateFn.trigger(); }; protected async onDispatch(payload: ActionPayload) { @@ -127,8 +130,12 @@ export class RoomListStore2 extends AsyncStore { // Update any settings here, as some may have happened before we were logically ready. console.log("Regenerating room lists: Startup"); await this.readAndCacheSettingsFromStore(); - await this.regenerateAllLists(); - this.onRVSUpdate(); // fake an RVS update to adjust sticky room, if needed + await this.regenerateAllLists(true); + await this.onRVSUpdate(true); // fake an RVS update to adjust sticky room, if needed + + this.updateFn.trigger(); + + return; // no point in running the next conditions - they won't match } // TODO: Remove this once the RoomListStore becomes default @@ -137,7 +144,7 @@ export class RoomListStore2 extends AsyncStore { if (payload.action === 'on_client_not_viable' || payload.action === 'on_logged_out') { // Reset state without causing updates as the client will have been destroyed // and downstream code will throw NPE errors. - this.reset(null, true); + await this.reset(null, true); this._matrixClient = null; this.initialListsGenerated = false; // we'll want to regenerate them } @@ -151,7 +158,8 @@ export class RoomListStore2 extends AsyncStore { console.log("Regenerating room lists: Settings changed"); await this.readAndCacheSettingsFromStore(); - await this.regenerateAllLists(); // regenerate the lists now + await this.regenerateAllLists(true); // regenerate the lists now + this.updateFn.trigger(); } } @@ -172,6 +180,7 @@ export class RoomListStore2 extends AsyncStore { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[RoomListDebug] Got own read receipt in ${room.roomId}`); await this.handleRoomUpdate(room, RoomUpdateCause.ReadReceipt); + this.updateFn.trigger(); return; } } else if (payload.action === 'MatrixActions.Room.tags') { @@ -179,6 +188,7 @@ export class RoomListStore2 extends AsyncStore { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[RoomListDebug] Got tag change in ${roomPayload.room.roomId}`); await this.handleRoomUpdate(roomPayload.room, RoomUpdateCause.PossibleTagChange); + this.updateFn.trigger(); } else if (payload.action === 'MatrixActions.Room.timeline') { const eventPayload = (payload); // TODO: Type out the dispatcher types @@ -202,6 +212,7 @@ export class RoomListStore2 extends AsyncStore { } } await this.handleRoomUpdate(updatedRoom, RoomUpdateCause.Timeline); + this.updateFn.trigger(); }; if (!room) { console.warn(`Live timeline event ${eventPayload.event.getId()} received without associated room`); @@ -225,6 +236,7 @@ export class RoomListStore2 extends AsyncStore { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[RoomListDebug] Decrypted timeline event ${eventPayload.event.getId()} in ${roomId}`); await this.handleRoomUpdate(room, RoomUpdateCause.Timeline); + this.updateFn.trigger(); } else if (payload.action === 'MatrixActions.accountData' && payload.event_type === 'm.direct') { const eventPayload = (payload); // TODO: Type out the dispatcher types // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 @@ -246,6 +258,7 @@ export class RoomListStore2 extends AsyncStore { await this.handleRoomUpdate(room, RoomUpdateCause.PossibleTagChange); } } + this.updateFn.trigger(); } else if (payload.action === 'MatrixActions.Room.myMembership') { const membershipPayload = (payload); // TODO: Type out the dispatcher types const oldMembership = getEffectiveMembership(membershipPayload.oldMembership); @@ -264,7 +277,7 @@ export class RoomListStore2 extends AsyncStore { const isSticky = this.algorithm.stickyRoom === prevRoom; if (isSticky) { console.log(`[RoomListDebug] Clearing sticky room due to room upgrade`); - await this.algorithm.setStickyRoomAsync(null); + await this.algorithm.setStickyRoom(null); } // Note: we hit the algorithm instead of our handleRoomUpdate() function to @@ -276,6 +289,7 @@ export class RoomListStore2 extends AsyncStore { console.log(`[RoomListDebug] Adding new room to room list`); await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.NewRoom); + this.updateFn.trigger(); return; } @@ -283,6 +297,7 @@ export class RoomListStore2 extends AsyncStore { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[RoomListDebug] Handling invite to ${membershipPayload.room.roomId}`); await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.NewRoom); + this.updateFn.trigger(); return; } @@ -291,6 +306,7 @@ export class RoomListStore2 extends AsyncStore { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[RoomListDebug] Handling membership change in ${membershipPayload.room.roomId}`); await this.handleRoomUpdate(membershipPayload.room, RoomUpdateCause.PossibleTagChange); + this.updateFn.trigger(); return; } } @@ -301,7 +317,7 @@ export class RoomListStore2 extends AsyncStore { if (shouldUpdate) { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[DEBUG] Room "${room.name}" (${room.roomId}) triggered by ${cause} requires list update`); - this.emit(LISTS_UPDATE_EVENT, this); + this.updateFn.mark(); } } @@ -309,6 +325,7 @@ export class RoomListStore2 extends AsyncStore { await this.algorithm.setTagSorting(tagId, sort); // TODO: Per-account? https://github.com/vector-im/riot-web/issues/14114 localStorage.setItem(`mx_tagSort_${tagId}`, sort); + this.updateFn.triggerIfWillMark(); } public getTagSorting(tagId: TagID): SortAlgorithm { @@ -347,6 +364,7 @@ export class RoomListStore2 extends AsyncStore { await this.algorithm.setListOrdering(tagId, order); // TODO: Per-account? https://github.com/vector-im/riot-web/issues/14114 localStorage.setItem(`mx_listOrder_${tagId}`, order); + this.updateFn.triggerIfWillMark(); } public getListOrder(tagId: TagID): ListAlgorithm { @@ -382,6 +400,10 @@ export class RoomListStore2 extends AsyncStore { } private async updateAlgorithmInstances() { + // We'll require an update, so mark for one. Marking now also prevents the calls + // to setTagSorting and setListOrder from causing triggers. + this.updateFn.mark(); + for (const tag of Object.keys(this.orderedLists)) { const definedSort = this.getTagSorting(tag); const definedOrder = this.getListOrder(tag); @@ -406,11 +428,11 @@ export class RoomListStore2 extends AsyncStore { private onAlgorithmListUpdated = () => { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log("Underlying algorithm has triggered a list update - refiring"); - this.emit(LISTS_UPDATE_EVENT, this); + console.log("Underlying algorithm has triggered a list update - marking"); + this.updateFn.mark(); }; - private async regenerateAllLists() { + private async regenerateAllLists(quiet = false) { console.warn("Regenerating all room lists"); const sorts: ITagSortingMap = {}; @@ -435,7 +457,7 @@ export class RoomListStore2 extends AsyncStore { this.initialListsGenerated = true; - this.emit(LISTS_UPDATE_EVENT, this); + if (!quiet) this.updateFn.trigger(); } public addFilter(filter: IFilterCondition): void { @@ -445,6 +467,7 @@ export class RoomListStore2 extends AsyncStore { if (this.algorithm) { this.algorithm.addFilterCondition(filter); } + this.updateFn.trigger(); } public removeFilter(filter: IFilterCondition): void { @@ -458,6 +481,7 @@ export class RoomListStore2 extends AsyncStore { this.algorithm.removeFilterCondition(filter); } } + this.updateFn.trigger(); } /** diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 35511a461d..2cd767682b 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -87,12 +87,6 @@ export class Algorithm extends EventEmitter { return this._stickyRoom ? this._stickyRoom.room : null; } - public set stickyRoom(val: Room) { - // setters can't be async, so we call a private function to do the work - // noinspection JSIgnoredPromiseFromCall - this.updateStickyRoom(val); - } - protected get hasFilters(): boolean { return this.allowedByFilter.size > 0; } @@ -115,7 +109,7 @@ export class Algorithm extends EventEmitter { * Awaitable version of the sticky room setter. * @param val The new room to sticky. */ - public async setStickyRoomAsync(val: Room) { + public async setStickyRoom(val: Room) { await this.updateStickyRoom(val); } @@ -746,7 +740,7 @@ export class Algorithm extends EventEmitter { }; } else { // We have to clear the lock as the sticky room change will trigger updates. - await this.setStickyRoomAsync(room); + await this.setStickyRoom(room); } } } diff --git a/src/utils/MarkedExecution.ts b/src/utils/MarkedExecution.ts new file mode 100644 index 0000000000..d866720ecd --- /dev/null +++ b/src/utils/MarkedExecution.ts @@ -0,0 +1,67 @@ +/* +Copyright 2020 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/** + * A utility to ensure that a function is only called once triggered with + * a mark applied. Multiple marks can be applied to the function, however + * the function will only be called once upon trigger(). + * + * The function starts unmarked. + */ +export class MarkedExecution { + private marked = false; + + /** + * Creates a MarkedExecution for the provided function. + * @param fn The function to be called upon trigger if marked. + */ + constructor(private fn: () => void) { + } + + /** + * Resets the mark without calling the function. + */ + public reset() { + this.marked = false; + } + + /** + * Marks the function to be called upon trigger(). + */ + public mark() { + this.marked = true; + } + + /** + * If marked, the function will be called, otherwise this does nothing. + */ + public trigger() { + if (!this.marked) return; + this.fn(); + this.reset(); + } + + /** + * Triggers the function if a mark() call would mark it. If the function + * has already been marked this will do nothing. + */ + public triggerIfWillMark() { + if (!this.marked) { + this.mark(); + this.trigger(); + } + } +} From 8624e8beeb66d75350aa4ea16176de1e4409a42d Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 9 Jul 2020 15:11:21 -0600 Subject: [PATCH 2/5] Break up the event loop tasks for the room list The room list does a hefty amount of work, so instead of blocking the event loop with a `/sync` request and a bunch of room updates (as we can get multiple per sync) we can instead run it over several smaller tasks. The smaller tasks help the event loop do other things between our tasks, ensuring we don't inadvertently block the UI from rendering too slowly. On my account and machine, this cuts the time to render in half (~30ms, down from ~60ms) . --- src/stores/room-list/RoomListStore2.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/stores/room-list/RoomListStore2.ts b/src/stores/room-list/RoomListStore2.ts index c29c0b5a20..60f888f8ed 100644 --- a/src/stores/room-list/RoomListStore2.ts +++ b/src/stores/room-list/RoomListStore2.ts @@ -114,7 +114,13 @@ export class RoomListStore2 extends AsyncStore { if (!quiet) this.updateFn.trigger(); }; - protected async onDispatch(payload: ActionPayload) { + protected onDispatch(payload: ActionPayload) { + // We do this to intentionally break out of the current event loop task, allowing + // us to instead wait for a more convenient time to run our updates. + setImmediate(() => this.onDispatchAsync(payload)); + } + + protected async onDispatchAsync(payload: ActionPayload) { if (payload.action === 'MatrixActions.sync') { // Filter out anything that isn't the first PREPARED sync. if (!(payload.prevState === 'PREPARED' && payload.state !== 'PREPARED')) { From a5ba0cad1fa7f2814c3c9127014ac3706d6e3fcf Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 10 Jul 2020 08:13:23 -0600 Subject: [PATCH 3/5] Rename to trigger and add docs --- src/stores/room-list/RoomListStore2.ts | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/stores/room-list/RoomListStore2.ts b/src/stores/room-list/RoomListStore2.ts index 5591de67b1..f1b26f26ce 100644 --- a/src/stores/room-list/RoomListStore2.ts +++ b/src/stores/room-list/RoomListStore2.ts @@ -92,7 +92,12 @@ export class RoomListStore2 extends AsyncStore { await this.updateAlgorithmInstances(); } - private onRVSUpdate = async (quiet = false) => { + /** + * Handles suspected RoomViewStore changes. + * @param trigger Set to false to prevent a list update from being sent. Should only + * be used if the calling code will manually trigger the update. + */ + private onRVSUpdate = async ({trigger = true}) => { if (!this.enabled) return; // TODO: Remove with https://github.com/vector-im/riot-web/issues/14231 if (!this.matrixClient) return; // We assume there won't be RVS updates without a client @@ -113,7 +118,7 @@ export class RoomListStore2 extends AsyncStore { } } - if (!quiet) this.updateFn.trigger(); + if (trigger) this.updateFn.trigger(); }; protected onDispatch(payload: ActionPayload) { @@ -138,8 +143,8 @@ export class RoomListStore2 extends AsyncStore { // Update any settings here, as some may have happened before we were logically ready. console.log("Regenerating room lists: Startup"); await this.readAndCacheSettingsFromStore(); - await this.regenerateAllLists(true); - await this.onRVSUpdate(true); // fake an RVS update to adjust sticky room, if needed + await this.regenerateAllLists({trigger: false}); + await this.onRVSUpdate({trigger: false}); // fake an RVS update to adjust sticky room, if needed this.updateFn.trigger(); @@ -166,7 +171,7 @@ export class RoomListStore2 extends AsyncStore { console.log("Regenerating room lists: Settings changed"); await this.readAndCacheSettingsFromStore(); - await this.regenerateAllLists(true); // regenerate the lists now + await this.regenerateAllLists({trigger: false}); // regenerate the lists now this.updateFn.trigger(); } } @@ -474,7 +479,12 @@ export class RoomListStore2 extends AsyncStore { this.updateFn.mark(); }; - private async regenerateAllLists(quiet = false) { + /** + * Regenerates the room whole room list, discarding any previous results. + * @param trigger Set to false to prevent a list update from being sent. Should only + * be used if the calling code will manually trigger the update. + */ + private async regenerateAllLists({trigger = true}) { console.warn("Regenerating all room lists"); const sorts: ITagSortingMap = {}; @@ -499,7 +509,7 @@ export class RoomListStore2 extends AsyncStore { this.initialListsGenerated = true; - if (!quiet) this.updateFn.trigger(); + if (trigger) this.updateFn.trigger(); } public addFilter(filter: IFilterCondition): void { From 46d53e5c8c55cb82a768eb438ce4edb9fcdc321d Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 10 Jul 2020 08:14:33 -0600 Subject: [PATCH 4/5] Reset before trigger just in case the function triggers too --- src/utils/MarkedExecution.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/MarkedExecution.ts b/src/utils/MarkedExecution.ts index d866720ecd..de6cf05953 100644 --- a/src/utils/MarkedExecution.ts +++ b/src/utils/MarkedExecution.ts @@ -50,8 +50,8 @@ export class MarkedExecution { */ public trigger() { if (!this.marked) return; + this.reset(); // reset first just in case the fn() causes a trigger() this.fn(); - this.reset(); } /** From 26427817f2704b674e5cea603786e3d968a53f27 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 10 Jul 2020 08:18:40 -0600 Subject: [PATCH 5/5] Fix potential listener conflict with RVS If the RVS ever emits something that contains `trigger: false`, we're pretty screwed, so avoid that. --- src/stores/room-list/RoomListStore2.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/stores/room-list/RoomListStore2.ts b/src/stores/room-list/RoomListStore2.ts index f1b26f26ce..05f678160e 100644 --- a/src/stores/room-list/RoomListStore2.ts +++ b/src/stores/room-list/RoomListStore2.ts @@ -63,7 +63,7 @@ export class RoomListStore2 extends AsyncStore { this.checkEnabled(); for (const settingName of this.watchedSettings) SettingsStore.monitorSetting(settingName, null); - RoomViewStore.addListener(this.onRVSUpdate); + RoomViewStore.addListener(() => this.handleRVSUpdate({})); this.algorithm.on(LIST_UPDATED_EVENT, this.onAlgorithmListUpdated); } @@ -97,7 +97,7 @@ export class RoomListStore2 extends AsyncStore { * @param trigger Set to false to prevent a list update from being sent. Should only * be used if the calling code will manually trigger the update. */ - private onRVSUpdate = async ({trigger = true}) => { + private async handleRVSUpdate({trigger = true}) { if (!this.enabled) return; // TODO: Remove with https://github.com/vector-im/riot-web/issues/14231 if (!this.matrixClient) return; // We assume there won't be RVS updates without a client @@ -119,7 +119,7 @@ export class RoomListStore2 extends AsyncStore { } if (trigger) this.updateFn.trigger(); - }; + } protected onDispatch(payload: ActionPayload) { // We do this to intentionally break out of the current event loop task, allowing @@ -144,7 +144,7 @@ export class RoomListStore2 extends AsyncStore { console.log("Regenerating room lists: Startup"); await this.readAndCacheSettingsFromStore(); await this.regenerateAllLists({trigger: false}); - await this.onRVSUpdate({trigger: false}); // fake an RVS update to adjust sticky room, if needed + await this.handleRVSUpdate({trigger: false}); // fake an RVS update to adjust sticky room, if needed this.updateFn.trigger();