Merge pull request #5047 from matrix-org/travis/perf3

Reward users for using stable ordering in their room list

src/components/views/rooms/RoomList.tsx

@@ -42,6 +42,8 @@ import { ViewRoomDeltaPayload } from "../../../dispatcher/payloads/ViewRoomDelta
 import { RoomNotificationStateStore } from "../../../stores/notifications/RoomNotificationStateStore";
 import SettingsStore from "../../../settings/SettingsStore";
 import CustomRoomTagStore from "../../../stores/CustomRoomTagStore";
+import { arrayFastClone, arrayHasDiff } from "../../../utils/arrays";
+import { objectShallowClone } from "../../../utils/objects";
 
 interface IProps {
     onKeyDown: (ev: React.KeyboardEvent) => void;
@@ -80,7 +82,7 @@ interface ITagAesthetics {
     sectionLabel: string;
     sectionLabelRaw?: string;
     addRoomLabel?: string;
-    onAddRoom?: (dispatcher: Dispatcher<ActionPayload>) => void;
+    onAddRoom?: (dispatcher?: Dispatcher<ActionPayload>) => void;
     isInvite: boolean;
     defaultHidden: boolean;
 }
@@ -104,14 +106,18 @@ const TAG_AESTHETICS: {
         isInvite: false,
         defaultHidden: false,
         addRoomLabel: _td("Start chat"),
-        onAddRoom: (dispatcher: Dispatcher<ActionPayload>) => dispatcher.dispatch({action: 'view_create_chat'}),
+        onAddRoom: (dispatcher?: Dispatcher<ActionPayload>) => {
+            (dispatcher || defaultDispatcher).dispatch({action: 'view_create_chat'});
+        },
     },
     [DefaultTagID.Untagged]: {
         sectionLabel: _td("Rooms"),
         isInvite: false,
         defaultHidden: false,
         addRoomLabel: _td("Create room"),
-        onAddRoom: (dispatcher: Dispatcher<ActionPayload>) => dispatcher.dispatch({action: 'view_create_room'}),
+        onAddRoom: (dispatcher?: Dispatcher<ActionPayload>) => {
+            (dispatcher || defaultDispatcher).dispatch({action: 'view_create_room'})
+        },
     },
     [DefaultTagID.LowPriority]: {
         sectionLabel: _td("Low priority"),
@@ -231,9 +237,33 @@ export default class RoomList extends React.Component<IProps, IState> {
             console.log("new lists", newLists);
         }
 
-        this.setState({sublists: newLists}, () => {
+        const previousListIds = Object.keys(this.state.sublists);
-            this.props.onResize();
+        const newListIds = Object.keys(newLists);
-        });
+
+        let doUpdate = arrayHasDiff(previousListIds, newListIds);
+        if (!doUpdate) {
+            // so we didn't have the visible sublists change, but did the contents of those
+            // sublists change significantly enough to break the sticky headers? Probably, so
+            // let's check the length of each.
+            for (const tagId of newListIds) {
+                const oldRooms = this.state.sublists[tagId];
+                const newRooms = newLists[tagId];
+                if (oldRooms.length !== newRooms.length) {
+                    doUpdate = true;
+                    break;
+                }
+            }
+        }
+
+        if (doUpdate) {
+            // We have to break our reference to the room list store if we want to be able to
+            // diff the object for changes, so do that.
+            const sublists = objectShallowClone(newLists, (k, v) => arrayFastClone(v));
+
+            this.setState({sublists}, () => {
+                this.props.onResize();
+            });
+        }
     };
 
     private renderCommunityInvites(): React.ReactElement[] {
@@ -298,16 +328,14 @@ export default class RoomList extends React.Component<IProps, IState> {
                 : TAG_AESTHETICS[orderedTagId];
             if (!aesthetics) throw new Error(`Tag ${orderedTagId} does not have aesthetics`);
 
-            const onAddRoomFn = aesthetics.onAddRoom ? () => aesthetics.onAddRoom(dis) : null;
             components.push(
                 <RoomSublist
                     key={`sublist-${orderedTagId}`}
                     tagId={orderedTagId}
                     forRooms={true}
-                    rooms={orderedRooms}
                     startAsHidden={aesthetics.defaultHidden}
                     label={aesthetics.sectionLabelRaw ? aesthetics.sectionLabelRaw : _t(aesthetics.sectionLabel)}
-                    onAddRoom={onAddRoomFn}
+                    onAddRoom={aesthetics.onAddRoom}
                     addRoomLabel={aesthetics.addRoomLabel ? _t(aesthetics.addRoomLabel) : aesthetics.addRoomLabel}
                     isMinimized={this.props.isMinimized}
                     onResize={this.props.onResize}

src/components/views/rooms/RoomSublist.tsx

@@ -32,7 +32,7 @@ import {
     StyledMenuItemCheckbox,
     StyledMenuItemRadio,
 } from "../../structures/ContextMenu";
-import RoomListStore from "../../../stores/room-list/RoomListStore";
+import RoomListStore, { LISTS_UPDATE_EVENT } from "../../../stores/room-list/RoomListStore";
 import { ListAlgorithm, SortAlgorithm } from "../../../stores/room-list/algorithms/models";
 import { DefaultTagID, TagID } from "../../../stores/room-list/models";
 import dis from "../../../dispatcher/dispatcher";
@@ -47,6 +47,8 @@ import { Direction } from "re-resizable/lib/resizer";
 import { polyfillTouchEvent } from "../../../@types/polyfill";
 import { RoomNotificationStateStore } from "../../../stores/notifications/RoomNotificationStateStore";
 import RoomListLayoutStore from "../../../stores/room-list/RoomListLayoutStore";
+import { arrayHasOrderChange } from "../../../utils/arrays";
+import { objectExcluding, objectHasValueChange } from "../../../utils/objects";
 
 const SHOW_N_BUTTON_HEIGHT = 28; // As defined by CSS
 const RESIZE_HANDLE_HEIGHT = 4; // As defined by CSS
@@ -59,7 +61,6 @@ polyfillTouchEvent();
 
 interface IProps {
     forRooms: boolean;
-    rooms?: Room[];
     startAsHidden: boolean;
     label: string;
     onAddRoom?: () => void;
@@ -90,6 +91,7 @@ interface IState {
     isResizing: boolean;
     isExpanded: boolean; // used for the for expand of the sublist when the room list is being filtered
     height: number;
+    rooms: Room[];
 }
 
 export default class RoomSublist extends React.Component<IProps, IState> {
@@ -104,16 +106,19 @@ export default class RoomSublist extends React.Component<IProps, IState> {
 
         this.layout = RoomListLayoutStore.instance.getLayoutFor(this.props.tagId);
         this.heightAtStart = 0;
-        const height = this.calculateInitialHeight();
         this.state = {
             notificationState: RoomNotificationStateStore.instance.getListState(this.props.tagId),
             contextMenuPosition: null,
             isResizing: false,
             isExpanded: this.props.isFiltered ? this.props.isFiltered : !this.layout.isCollapsed,
-            height,
+            height: 0, // to be fixed in a moment, we need `rooms` to calculate this.
+            rooms: RoomListStore.instance.orderedLists[this.props.tagId] || [],
         };
-        this.state.notificationState.setRooms(this.props.rooms);
+        // Why Object.assign() and not this.state.height? Because TypeScript says no.
+        this.state = Object.assign(this.state, {height: this.calculateInitialHeight()});
+        this.state.notificationState.setRooms(this.state.rooms);
         this.dispatcherRef = defaultDispatcher.register(this.onAction);
+        RoomListStore.instance.on(LISTS_UPDATE_EVENT, this.onListsUpdated);
     }
 
     private calculateInitialHeight() {
@@ -142,11 +147,11 @@ export default class RoomSublist extends React.Component<IProps, IState> {
     }
 
     private get numTiles(): number {
-        return RoomSublist.calcNumTiles(this.props);
+        return RoomSublist.calcNumTiles(this.state.rooms, this.props.extraBadTilesThatShouldntExist);
     }
 
-    private static calcNumTiles(props) {
+    private static calcNumTiles(rooms: Room[], extraTiles: any[]) {
-        return (props.rooms || []).length + (props.extraBadTilesThatShouldntExist || []).length;
+        return (rooms || []).length + (extraTiles || []).length;
     }
 
     private get numVisibleTiles(): number {
@@ -154,8 +159,8 @@ export default class RoomSublist extends React.Component<IProps, IState> {
         return Math.min(nVisible, this.numTiles);
     }
 
-    public componentDidUpdate(prevProps: Readonly<IProps>) {
+    public componentDidUpdate(prevProps: Readonly<IProps>, prevState: Readonly<IState>) {
-        this.state.notificationState.setRooms(this.props.rooms);
+        this.state.notificationState.setRooms(this.state.rooms);
         if (prevProps.isFiltered !== this.props.isFiltered) {
             if (this.props.isFiltered) {
                 this.setState({isExpanded: true});
@@ -165,22 +170,87 @@ export default class RoomSublist extends React.Component<IProps, IState> {
         }
         // as the rooms can come in one by one we need to reevaluate
         // the amount of available rooms to cap the amount of requested visible rooms by the layout
-        if (RoomSublist.calcNumTiles(prevProps) !== this.numTiles) {
+        if (RoomSublist.calcNumTiles(prevState.rooms, prevProps.extraBadTilesThatShouldntExist) !== this.numTiles) {
             this.setState({height: this.calculateInitialHeight()});
         }
     }
 
+    public shouldComponentUpdate(nextProps: Readonly<IProps>, nextState: Readonly<IState>): boolean {
+        if (objectHasValueChange(this.props, nextProps)) {
+            // Something we don't care to optimize has updated, so update.
+            return true;
+        }
+
+        // Do the same check used on props for state, without the rooms we're going to no-op
+        const prevStateNoRooms = objectExcluding(this.state, ['rooms']);
+        const nextStateNoRooms = objectExcluding(nextState, ['rooms']);
+        if (objectHasValueChange(prevStateNoRooms, nextStateNoRooms)) {
+            return true;
+        }
+
+        // If we're supposed to handle extra tiles, take the performance hit and re-render all the
+        // time so we don't have to consider them as part of the visible room optimization.
+        const prevExtraTiles = this.props.extraBadTilesThatShouldntExist || [];
+        const nextExtraTiles = nextProps.extraBadTilesThatShouldntExist || [];
+        if (prevExtraTiles.length > 0 || nextExtraTiles.length > 0) {
+            return true;
+        }
+
+        // If we're about to update the height of the list, we don't really care about which rooms
+        // are visible or not for no-op purposes, so ensure that the height calculation runs through.
+        if (RoomSublist.calcNumTiles(nextState.rooms, nextProps.extraBadTilesThatShouldntExist) !== this.numTiles) {
+            return true;
+        }
+
+        // Before we go analyzing the rooms, we can see if we're collapsed. If we're collapsed, we don't need
+        // to render anything. We do this after the height check though to ensure that the height gets appropriately
+        // calculated for when/if we become uncollapsed.
+        if (!nextState.isExpanded) {
+            return false;
+        }
+
+        // Quickly double check we're not about to break something due to the number of rooms changing.
+        if (this.state.rooms.length !== nextState.rooms.length) {
+            return true;
+        }
+
+        // Finally, determine if the room update (as presumably that's all that's left) is within
+        // our visible range. If it is, then do a render. If the update is outside our visible range
+        // then we can skip the update.
+        //
+        // We also optimize for order changing here: if the update did happen in our visible range
+        // but doesn't result in the list re-sorting itself then there's no reason for us to update
+        // on our own.
+        const prevSlicedRooms = this.state.rooms.slice(0, this.numVisibleTiles);
+        const nextSlicedRooms = nextState.rooms.slice(0, this.numVisibleTiles);
+        if (arrayHasOrderChange(prevSlicedRooms, nextSlicedRooms)) {
+            return true;
+        }
+
+        // Finally, nothing happened so no-op the update
+        return false;
+    }
+
     public componentWillUnmount() {
         this.state.notificationState.destroy();
         defaultDispatcher.unregister(this.dispatcherRef);
+        RoomListStore.instance.off(LISTS_UPDATE_EVENT, this.onListsUpdated);
     }
 
+    private onListsUpdated = () => {
+        const currentRooms = this.state.rooms;
+        const newRooms = RoomListStore.instance.orderedLists[this.props.tagId] || [];
+        if (arrayHasOrderChange(currentRooms, newRooms)) {
+            this.setState({rooms: newRooms});
+        }
+    };
+
     private onAction = (payload: ActionPayload) => {
-        if (payload.action === "view_room" && payload.show_room_tile && this.props.rooms) {
+        if (payload.action === "view_room" && payload.show_room_tile && this.state.rooms) {
             // XXX: we have to do this a tick later because we have incorrect intermediate props during a room change
             // where we lose the room we are changing from temporarily and then it comes back in an update right after.
             setImmediate(() => {
-                const roomIndex = this.props.rooms.findIndex((r) => r.roomId === payload.room_id);
+                const roomIndex = this.state.rooms.findIndex((r) => r.roomId === payload.room_id);
 
                 if (!this.state.isExpanded && roomIndex > -1) {
                     this.toggleCollapsed();
@@ -302,10 +372,10 @@ export default class RoomSublist extends React.Component<IProps, IState> {
         let room;
         if (this.props.tagId === DefaultTagID.Invite) {
             // switch to first room as that'll be the top of the list for the user
-            room = this.props.rooms && this.props.rooms[0];
+            room = this.state.rooms && this.state.rooms[0];
         } else {
             // find the first room with a count of the same colour as the badge count
-            room = this.props.rooms.find((r: Room) => {
+            room = this.state.rooms.find((r: Room) => {
                 const notifState = this.state.notificationState.getForRoom(r);
                 return notifState.count > 0 && notifState.color === this.state.notificationState.color;
             });
@@ -399,8 +469,8 @@ export default class RoomSublist extends React.Component<IProps, IState> {
 
         const tiles: React.ReactElement[] = [];
 
-        if (this.props.rooms) {
+        if (this.state.rooms) {
-            const visibleRooms = this.props.rooms.slice(0, this.numVisibleTiles);
+            const visibleRooms = this.state.rooms.slice(0, this.numVisibleTiles);
             for (const room of visibleRooms) {
                 tiles.push(
                     <RoomTile

src/components/views/rooms/RoomTile.tsx

@@ -35,7 +35,7 @@ import {
     MenuItem,
 } from "../../structures/ContextMenu";
 import { DefaultTagID, TagID } from "../../../stores/room-list/models";
-import { MessagePreviewStore } from "../../../stores/room-list/MessagePreviewStore";
+import { MessagePreviewStore, ROOM_PREVIEW_CHANGED } from "../../../stores/room-list/MessagePreviewStore";
 import DecoratedRoomAvatar from "../avatars/DecoratedRoomAvatar";
 import {
     getRoomNotifsState,
@@ -128,6 +128,7 @@ export default class RoomTile extends React.Component<IProps, IState> {
 
         ActiveRoomObserver.addListener(this.props.room.roomId, this.onActiveRoomUpdate);
         this.dispatcherRef = defaultDispatcher.register(this.onAction);
+        MessagePreviewStore.instance.on(ROOM_PREVIEW_CHANGED, this.onRoomPreviewChanged);
     }
 
     private get showContextMenu(): boolean {
@@ -150,6 +151,7 @@ export default class RoomTile extends React.Component<IProps, IState> {
             ActiveRoomObserver.removeListener(this.props.room.roomId, this.onActiveRoomUpdate);
         }
         defaultDispatcher.unregister(this.dispatcherRef);
+        MessagePreviewStore.instance.off(ROOM_PREVIEW_CHANGED, this.onRoomPreviewChanged);
     }
 
     private onAction = (payload: ActionPayload) => {
@@ -160,6 +162,12 @@ export default class RoomTile extends React.Component<IProps, IState> {
         }
     };
 
+    private onRoomPreviewChanged = (room: Room) => {
+        if (this.props.room && room.roomId === this.props.room.roomId) {
+            this.forceUpdate(); // we don't have any state to set, so just complain that we need an update
+        }
+    };
+
     private scrollIntoView = () => {
         if (!this.roomTileRef.current) return;
         this.roomTileRef.current.scrollIntoView({

src/stores/room-list/MessagePreviewStore.ts

@@ -28,6 +28,10 @@ import { StickerEventPreview } from "./previews/StickerEventPreview";
 import { ReactionEventPreview } from "./previews/ReactionEventPreview";
 import { UPDATE_EVENT } from "../AsyncStore";
 
+// Emitted event for when a room's preview has changed. First argument will the room for which
+// the change happened.
+export const ROOM_PREVIEW_CHANGED = "room_preview_changed";
+
 const PREVIEWS = {
     'm.room.message': {
         isState: false,
@@ -146,6 +150,7 @@ export class MessagePreviewStore extends AsyncStoreWithClient<IState> {
                 // We've muted the underlying Map, so just emit that we've changed.
                 this.previews.set(room.roomId, map);
                 this.emit(UPDATE_EVENT, this);
+                this.emit(ROOM_PREVIEW_CHANGED, room);
             }
             return; // we're done
         }
@@ -153,6 +158,7 @@ export class MessagePreviewStore extends AsyncStoreWithClient<IState> {
         // At this point, we didn't generate a preview so clear it
         this.previews.set(room.roomId, new Map<TagID|TAG_ANY, string|null>());
         this.emit(UPDATE_EVENT, this);
+        this.emit(ROOM_PREVIEW_CHANGED, room);
     }
 
     protected async onAction(payload: ActionPayload) {

src/utils/arrays.ts

@@ -14,11 +14,38 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+/**
+ * Clones an array as fast as possible, retaining references of the array's values.
+ * @param a The array to clone. Must be defined.
+ * @returns A copy of the array.
+ */
+export function arrayFastClone(a: any[]): any[] {
+    return a.slice(0, a.length);
+}
+
+/**
+ * Determines if the two arrays are different either in length, contents,
+ * or order of those contents.
+ * @param a The first array. Must be defined.
+ * @param b The second array. Must be defined.
+ * @returns True if they are different, false otherwise.
+ */
+export function arrayHasOrderChange(a: any[], b: any[]): boolean {
+    if (a.length === b.length) {
+        for (let i = 0; i < a.length; i++) {
+            if (a[i] !== b[i]) return true;
+        }
+        return false;
+    } else {
+        return true; // like arrayHasDiff, a difference in length is a natural change
+    }
+}
+
 /**
  * Determines if two arrays are different through a shallow comparison.
  * @param a The first array. Must be defined.
  * @param b The second array. Must be defined.
- * @returns True if they are the same, false otherwise.
+ * @returns True if they are different, false otherwise.
  */
 export function arrayHasDiff(a: any[], b: any[]): boolean {
     if (a.length === b.length) {

src/utils/objects.ts

@@ -14,7 +14,63 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { arrayDiff, arrayMerge, arrayUnion } from "./arrays";
+import { arrayDiff, arrayHasDiff, arrayMerge, arrayUnion } from "./arrays";
+
+/**
+ * Gets a new object which represents the provided object, excluding some properties.
+ * @param a The object to strip properties of. Must be defined.
+ * @param props The property names to remove.
+ * @returns The new object without the provided properties.
+ */
+export function objectExcluding(a: any, props: string[]): any {
+    // We use a Map to avoid hammering the `delete` keyword, which is slow and painful.
+    const tempMap = new Map<string, any>(Object.entries(a));
+    for (const prop of props) {
+        tempMap.delete(prop);
+    }
+
+    // Convert the map to an object again
+    return Array.from(tempMap.entries()).reduce((c, [k, v]) => {
+        c[k] = v;
+        return c;
+    }, {});
+}
+
+/**
+ * Clones an object to a caller-controlled depth. When a propertyCloner is supplied, the
+ * object's properties will be passed through it with the return value used as the new
+ * object's type. This is intended to be used to deep clone a reference, but without
+ * having to deep clone the entire object. This function is safe to call recursively within
+ * the propertyCloner.
+ * @param a The object to clone. Must be defined.
+ * @param propertyCloner The function to clone the properties of the object with, optionally.
+ * First argument is the property key with the second being the current value.
+ * @returns A cloned object.
+ */
+export function objectShallowClone(a: any, propertyCloner?: (k: string, v: any) => any): any {
+    const newObj = {};
+    for (const [k, v] of Object.entries(a)) {
+        newObj[k] = v;
+        if (propertyCloner) {
+            newObj[k] = propertyCloner(k, v);
+        }
+    }
+    return newObj;
+}
+
+/**
+ * Determines if the two objects, which are assumed to be of the same
+ * key shape, have a difference in their values. If a difference is
+ * determined, true is returned.
+ * @param a The first object. Must be defined.
+ * @param b The second object. Must be defined.
+ * @returns True if there's a perceptual difference in the object's values.
+ */
+export function objectHasValueChange(a: any, b: any): boolean {
+    const aValues = Object.values(a);
+    const bValues = Object.values(b);
+    return arrayHasDiff(aValues, bValues);
+}
 
 /**
  * Determines the keys added, changed, and removed between two objects.