From 102c4e5ae98e712c41d28a99428e623a9dadb1de Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 10 Mar 2023 14:35:40 +0000 Subject: [PATCH] Improve performance of rendering a room with many hidden events (#10131) * Test MessagePanel rendering with hidden events * Cache the results of shouldShowEvent in MessagePanel * Combine an event and its shouldShow info into a single object --- src/components/structures/MessagePanel.tsx | 73 ++++++++++++++----- .../structures/MessagePanel-test.tsx | 21 ++++++ .../__snapshots__/MessagePanel-test.tsx.snap | 21 ++++++ 3 files changed, 98 insertions(+), 17 deletions(-) create mode 100644 test/components/structures/__snapshots__/MessagePanel-test.tsx.snap diff --git a/src/components/structures/MessagePanel.tsx b/src/components/structures/MessagePanel.tsx index 3bfbe8e9c5..434b3ddd6e 100644 --- a/src/components/structures/MessagePanel.tsx +++ b/src/components/structures/MessagePanel.tsx @@ -561,14 +561,29 @@ export default class MessagePanel extends React.Component { }); }; - private getNextEventInfo(arr: MatrixEvent[], i: number): { nextEvent: MatrixEvent; nextTile: MatrixEvent } { - const nextEvent = i < arr.length - 1 ? arr[i + 1] : null; + /** + * Find the next event in the list, and the next visible event in the list. + * + * @param event - the list of events to look in and whether they are shown + * @param i - where in the list we are now + * + * @returns { nextEvent, nextTile } + * + * nextEvent is the event after i in the supplied array. + * + * nextTile is the first event in the array after i that we will show a tile + * for. It is used to to determine the 'last successful' flag when rendering + * the tile. + */ + private getNextEventInfo( + events: EventAndShouldShow[], + i: number, + ): { nextEvent: MatrixEvent | null; nextTile: MatrixEvent | null } { + // WARNING: this method is on a hot path. - // The next event with tile is used to to determine the 'last successful' flag - // when rendering the tile. The shouldShowEvent function is pretty quick at what - // it does, so this should have no significant cost even when a room is used for - // not-chat purposes. - const nextTile = arr.slice(i + 1).find((e) => this.shouldShowEvent(e)); + const nextEvent = i < events.length - 1 ? events[i + 1].event : null; + + const nextTile = findFirstShownAfter(i, events); return { nextEvent, nextTile }; } @@ -587,20 +602,21 @@ export default class MessagePanel extends React.Component { } private getEventTiles(): ReactNode[] { - let i; - // first figure out which is the last event in the list which we're // actually going to show; this allows us to behave slightly // differently for the last event in the list. (eg show timestamp) // // we also need to figure out which is the last event we show which isn't // a local echo, to manage the read-marker. - let lastShownEvent; + let lastShownEvent: MatrixEvent | undefined; + const events: EventAndShouldShow[] = this.props.events.map((event) => { + return { event, shouldShow: this.shouldShowEvent(event) }; + }); let lastShownNonLocalEchoIndex = -1; - for (i = this.props.events.length - 1; i >= 0; i--) { - const mxEv = this.props.events[i]; - if (!this.shouldShowEvent(mxEv)) { + for (let i = events.length - 1; i >= 0; i--) { + const { event: mxEv, shouldShow } = events[i]; + if (!shouldShow) { continue; } @@ -631,11 +647,11 @@ export default class MessagePanel extends React.Component { let grouper: BaseGrouper = null; - for (i = 0; i < this.props.events.length; i++) { - const mxEv = this.props.events[i]; + for (let i = 0; i < events.length; i++) { + const { event: mxEv, shouldShow } = events[i]; const eventId = mxEv.getId(); const last = mxEv === lastShownEvent; - const { nextEvent, nextTile } = this.getNextEventInfo(this.props.events, i); + const { nextEvent, nextTile } = this.getNextEventInfo(events, i); if (grouper) { if (grouper.shouldGroup(mxEv)) { @@ -658,7 +674,7 @@ export default class MessagePanel extends React.Component { } if (!grouper) { - if (this.shouldShowEvent(mxEv)) { + if (shouldShow) { // make sure we unpack the array returned by getTilesForEvent, // otherwise React will auto-generate keys, and we will end up // replacing all the DOM elements every time we paginate. @@ -1040,6 +1056,15 @@ export default class MessagePanel extends React.Component { } } +/** + * Holds on to an event, caching the information about whether it should be + * shown. Avoids calling shouldShowEvent more times than we need to. + */ +interface EventAndShouldShow { + event: MatrixEvent; + shouldShow: boolean; +} + abstract class BaseGrouper { public static canStartGroup = (panel: MessagePanel, ev: MatrixEvent): boolean => true; @@ -1369,3 +1394,17 @@ class MainGrouper extends BaseGrouper { // all the grouper classes that we use, ordered by priority const groupers = [CreationGrouper, MainGrouper]; + +/** + * Look through the supplied list of EventAndShouldShow, and return the first + * event that is >start items through the list, and is shown. + */ +function findFirstShownAfter(start: number, events: EventAndShouldShow[]): MatrixEvent | null { + for (let n = start + 1; n < events.length; n++) { + const { event, shouldShow } = events[n]; + if (shouldShow) { + return event; + } + } + return null; +} diff --git a/test/components/structures/MessagePanel-test.tsx b/test/components/structures/MessagePanel-test.tsx index b565cef3bc..7159c18e4d 100644 --- a/test/components/structures/MessagePanel-test.tsx +++ b/test/components/structures/MessagePanel-test.tsx @@ -669,6 +669,27 @@ describe("MessagePanel", function () { expect(els.length).toEqual(1); expect(els[0].getAttribute("data-scroll-tokens")?.split(",")).toHaveLength(3); }); + + it("should handle large numbers of hidden events quickly", () => { + // Increase the length of the loop here to test performance issues with + // rendering + + const events = []; + for (let i = 0; i < 100; i++) { + events.push( + TestUtilsMatrix.mkEvent({ + event: true, + type: "unknown.event.type", + content: { key: "value" }, + room: "!room:id", + user: "@user:id", + ts: 1000000 + i, + }), + ); + } + const { asFragment } = render(getComponent({ events }, { showHiddenEvents: false })); + expect(asFragment()).toMatchSnapshot(); + }); }); describe("shouldFormContinuation", () => { diff --git a/test/components/structures/__snapshots__/MessagePanel-test.tsx.snap b/test/components/structures/__snapshots__/MessagePanel-test.tsx.snap new file mode 100644 index 0000000000..4397e05860 --- /dev/null +++ b/test/components/structures/__snapshots__/MessagePanel-test.tsx.snap @@ -0,0 +1,21 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`MessagePanel should handle large numbers of hidden events quickly 1`] = ` + +
+
+
    +
+
+ ); + +`;