From d593d24aeabe9ff3f896fa74f031e8910d560600 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 22 Jul 2020 10:50:54 -0600 Subject: [PATCH 1/5] Switch to an internal Map for previews This means we're abusing the AsyncStoreWithClient to get access to a lifecycle, but overall that seems like a minor crime compared to the time spend abusing the store's state as a map. With thousands of rooms shown, we can save on average 743ms per preview. The new preview time is 0.12ms on average. --- src/stores/room-list/MessagePreviewStore.ts | 25 +++++++++++---------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/stores/room-list/MessagePreviewStore.ts b/src/stores/room-list/MessagePreviewStore.ts index 45f1b6c879..4a308f67b5 100644 --- a/src/stores/room-list/MessagePreviewStore.ts +++ b/src/stores/room-list/MessagePreviewStore.ts @@ -26,6 +26,7 @@ import { CallAnswerEventPreview } from "./previews/CallAnswerEventPreview"; import { CallHangupEvent } from "./previews/CallHangupEvent"; import { StickerEventPreview } from "./previews/StickerEventPreview"; import { ReactionEventPreview } from "./previews/ReactionEventPreview"; +import { UPDATE_EVENT } from "../AsyncStore"; const PREVIEWS = { 'm.room.message': { @@ -62,12 +63,15 @@ type TAG_ANY = "im.vector.any"; const TAG_ANY: TAG_ANY = "im.vector.any"; interface IState { - [roomId: string]: Map; // null indicates the preview is empty / irrelevant + // Empty because we don't actually use the state } export class MessagePreviewStore extends AsyncStoreWithClient { private static internalInstance = new MessagePreviewStore(); + // null indicates the preview is empty / irrelevant + private previews = new Map>(); + private constructor() { super(defaultDispatcher, {}); } @@ -85,10 +89,9 @@ export class MessagePreviewStore extends AsyncStoreWithClient { public getPreviewForRoom(room: Room, inTagId: TagID): string { if (!room) return null; // invalid room, just return nothing - const val = this.state[room.roomId]; - if (!val) this.generatePreview(room, inTagId); + if (!this.previews.has(room.roomId)) this.generatePreview(room, inTagId); - const previews = this.state[room.roomId]; + const previews = this.previews.get(room.roomId); if (!previews) return null; if (!previews.has(inTagId)) { @@ -101,11 +104,10 @@ export class MessagePreviewStore extends AsyncStoreWithClient { const events = room.timeline; if (!events) return; // should only happen in tests - let map = this.state[room.roomId]; + let map = this.previews.get(room.roomId); if (!map) { map = new Map(); - - // We set the state later with the map, so no need to send an update now + this.previews.set(room.roomId, map); } // Set the tags so we know what to generate @@ -141,16 +143,15 @@ export class MessagePreviewStore extends AsyncStoreWithClient { } if (changed) { - // Update state for good measure - causes emit for update - // noinspection JSIgnoredPromiseFromCall - the AsyncStore handles concurrent calls - this.updateState({[room.roomId]: map}); + // We've muted the underlying Map, so just emit that we've changed. + this.emit(UPDATE_EVENT, this); } return; // we're done } // At this point, we didn't generate a preview so clear it - // noinspection JSIgnoredPromiseFromCall - the AsyncStore handles concurrent calls - this.updateState({[room.roomId]: null}); + this.previews.delete(room.roomId); + this.emit(UPDATE_EVENT, this); } protected async onAction(payload: ActionPayload) { From 0ef6696c0a69ec39a04b3e4239807c17ab8fdb7c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 22 Jul 2020 10:52:45 -0600 Subject: [PATCH 2/5] Don't re-freeze AsyncStore's state all the time --- src/stores/AsyncStore.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/stores/AsyncStore.ts b/src/stores/AsyncStore.ts index 1977e808dc..1f310f003c 100644 --- a/src/stores/AsyncStore.ts +++ b/src/stores/AsyncStore.ts @@ -42,7 +42,7 @@ export const UPDATE_EVENT = "update"; * help prevent lock conflicts. */ export abstract class AsyncStore extends EventEmitter { - private storeState: T; + private storeState: Readonly; private lock = new AwaitLock(); private readonly dispatcherRef: string; @@ -62,7 +62,7 @@ export abstract class AsyncStore extends EventEmitter { * The current state of the store. Cannot be mutated. */ protected get state(): T { - return Object.freeze(this.storeState); + return this.storeState; } /** @@ -79,7 +79,7 @@ export abstract class AsyncStore extends EventEmitter { protected async updateState(newState: T | Object) { await this.lock.acquireAsync(); try { - this.storeState = Object.assign({}, this.storeState, newState); + this.storeState = Object.freeze(Object.assign({}, this.storeState, newState)); this.emit(UPDATE_EVENT, this); } finally { await this.lock.release(); @@ -94,7 +94,7 @@ export abstract class AsyncStore extends EventEmitter { protected async reset(newState: T | Object = null, quiet = false) { await this.lock.acquireAsync(); try { - this.storeState = (newState || {}); + this.storeState = Object.freeze((newState || {})); if (!quiet) this.emit(UPDATE_EVENT, this); } finally { await this.lock.release(); From f27afc6ff896a36260b00544f23249e609967830 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 22 Jul 2020 12:43:10 -0600 Subject: [PATCH 3/5] Fix message previews not updating on their own --- src/stores/room-list/MessagePreviewStore.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/stores/room-list/MessagePreviewStore.ts b/src/stores/room-list/MessagePreviewStore.ts index 4a308f67b5..f61488c3bb 100644 --- a/src/stores/room-list/MessagePreviewStore.ts +++ b/src/stores/room-list/MessagePreviewStore.ts @@ -144,13 +144,14 @@ export class MessagePreviewStore extends AsyncStoreWithClient { if (changed) { // We've muted the underlying Map, so just emit that we've changed. + this.previews.set(room.roomId, map); this.emit(UPDATE_EVENT, this); } return; // we're done } // At this point, we didn't generate a preview so clear it - this.previews.delete(room.roomId); + this.previews.set(room.roomId, new Map()); this.emit(UPDATE_EVENT, this); } @@ -159,7 +160,7 @@ export class MessagePreviewStore extends AsyncStoreWithClient { if (payload.action === 'MatrixActions.Room.timeline' || payload.action === 'MatrixActions.Event.decrypted') { const event = payload.event; // TODO: Type out the dispatcher - if (!Object.keys(this.state).includes(event.getRoomId())) return; // not important + if (!this.previews.has(event.getRoomId())) return; // not important this.generatePreview(this.matrixClient.getRoom(event.getRoomId()), TAG_ANY); } } From c22cb6c32576b1aa73a88626168938520a6eac35 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 22 Jul 2020 13:35:41 -0600 Subject: [PATCH 4/5] Short-circuit room list store dispatch handling if not ready We were taking 0.2ms to handle the registration of a timer per event during startup, even before the app is visible to the user. These timers would be short-circuited too, leading to a bunch of wasted time. 0.2ms isn't a lot of time, but multiplied by thousands of events at startup it's pretty significant. On my account this reduces the full page spinner time from ~50 seconds to just over 20 seconds. --- src/stores/room-list/RoomListStore.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/stores/room-list/RoomListStore.ts b/src/stores/room-list/RoomListStore.ts index 308296e8bb..1f6c14ba2f 100644 --- a/src/stores/room-list/RoomListStore.ts +++ b/src/stores/room-list/RoomListStore.ts @@ -168,6 +168,12 @@ export class RoomListStoreClass extends AsyncStoreWithClient { } protected async onAction(payload: ActionPayload) { + // If we're not remotely ready, don't even bother scheduling the dispatch handling. + // This is repeated in the handler just in case things change between a decision here and + // when the timer fires. + const logicallyReady = this.matrixClient && this.initialListsGenerated; + if (!logicallyReady) return; + // When we're running tests we can't reliably use setImmediate out of timing concerns. // As such, we use a more synchronous model. if (RoomListStoreClass.TEST_MODE) { From 80687e358f7103c8c361db3cd11d36bf754f63c5 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 22 Jul 2020 16:34:33 -0600 Subject: [PATCH 5/5] Obliterate some flexboxes in the room list We don't need columns of divs to equally size themselves, so use easier layout techniques to make the list fit in the container. We have to take a hit with `height:100%`, but the hit is much more insignificant than confusing the layout engine. The layout engine has a hard time with dynamically-but-statically-sized stuff like `width: 100%; display: flex;`, particularly when it is nested so badly. Overall this should improve performance for the app by not having to re-paint so often. Fixes https://github.com/vector-im/riot-web/issues/14639 --- res/css/structures/_LeftPanel.scss | 13 +------------ res/css/views/rooms/_RoomList.scss | 8 +------- res/css/views/rooms/_RoomSublist.scss | 7 ------- 3 files changed, 2 insertions(+), 26 deletions(-) diff --git a/res/css/structures/_LeftPanel.scss b/res/css/structures/_LeftPanel.scss index 1673092c9e..db531cf088 100644 --- a/res/css/structures/_LeftPanel.scss +++ b/res/css/structures/_LeftPanel.scss @@ -135,12 +135,7 @@ $tagPanelWidth: 56px; // only applies in this file, used for calculations } .mx_LeftPanel_roomListWrapper { - // Create a flexbox to ensure the containing items cause appropriate overflow. - display: flex; - - flex-grow: 1; overflow: hidden; - min-height: 0; margin-top: 10px; // so we're not up against the search/filter &.mx_LeftPanel_roomListWrapper_stickyBottom { @@ -153,14 +148,8 @@ $tagPanelWidth: 56px; // only applies in this file, used for calculations } .mx_LeftPanel_actualRoomListContainer { - flex-grow: 1; // fill the available space - overflow-y: auto; - width: 100%; - max-width: 100%; position: relative; // for sticky headers - - // Create a flexbox to trick the layout engine - display: flex; + height: 100%; // ensure scrolling still works } } diff --git a/res/css/views/rooms/_RoomList.scss b/res/css/views/rooms/_RoomList.scss index 690ed0646e..89ab85e146 100644 --- a/res/css/views/rooms/_RoomList.scss +++ b/res/css/views/rooms/_RoomList.scss @@ -15,11 +15,5 @@ limitations under the License. */ .mx_RoomList { - width: calc(100% - 16px); // 16px of artificial right-side margin (8px is overflowed from the sublists) - - // Create a column-based flexbox for the sublists. That's pretty much all we have to - // worry about in this stylesheet. - display: flex; - flex-direction: column; - flex-wrap: nowrap; // let the column overflow + padding-right: 7px; // width of the scrollbar, to line things up } diff --git a/res/css/views/rooms/_RoomSublist.scss b/res/css/views/rooms/_RoomSublist.scss index 3ec4d114af..b907d06d36 100644 --- a/res/css/views/rooms/_RoomSublist.scss +++ b/res/css/views/rooms/_RoomSublist.scss @@ -15,15 +15,8 @@ limitations under the License. */ .mx_RoomSublist { - // The sublist is a column of rows, essentially - display: flex; - flex-direction: column; - margin-left: 8px; margin-bottom: 4px; - width: 100%; - - flex-shrink: 0; // to convince safari's layout engine the flexbox is fine .mx_RoomSublist_headerContainer { // Create a flexbox to make alignment easy