From a3a431102e33dd6b59aadc364683a229ea1f4fb6 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 25 Feb 2022 17:27:25 +0000 Subject: [PATCH 1/2] Fix another freeze on room switch This switches permalinks to use the batch state update event and removes the incremental updates, as commented. We now spend, on my profiling, about 450ms in setOutOfBandMembers itself and another 120ms in permalinks. Fixes https://github.com/vector-im/element-web/issues/21127 --- src/utils/permalinks/Permalinks.ts | 80 ++++++++---------------- test/utils/permalinks/Permalinks-test.js | 6 +- 2 files changed, 28 insertions(+), 58 deletions(-) diff --git a/src/utils/permalinks/Permalinks.ts b/src/utils/permalinks/Permalinks.ts index 6e8d3fd915f..b8349f5fbf8 100644 --- a/src/utils/permalinks/Permalinks.ts +++ b/src/utils/permalinks/Permalinks.ts @@ -15,12 +15,8 @@ limitations under the License. */ import isIp from "is-ip"; -import { throttle } from "lodash"; import * as utils from "matrix-js-sdk/src/utils"; import { Room } from "matrix-js-sdk/src/models/room"; -import { EventType } from "matrix-js-sdk/src/@types/event"; -import { MatrixEvent } from "matrix-js-sdk/src/models/event"; -import { RoomMember, RoomMemberEvent } from "matrix-js-sdk/src/models/room-member"; import { logger } from "matrix-js-sdk/src/logger"; import { RoomStateEvent } from "matrix-js-sdk/src/models/room-state"; @@ -108,15 +104,9 @@ export class RoomPermalinkCreator { if (!this.roomId) { throw new Error("Failed to resolve a roomId for the permalink creator to use"); } - - if (shouldThrottle) { - this.updateServerCandidates = throttle( - this.updateServerCandidates, 200, { leading: true, trailing: true }, - ); - } } - load() { + public load() { if (!this.room || !this.room.currentState) { // Under rare and unknown circumstances it is possible to have a room with no // currentState, at least potentially at the early stages of joining a room. @@ -125,38 +115,33 @@ export class RoomPermalinkCreator { logger.warn("Tried to load a permalink creator with no room state"); return; } - this.updateAllowedServers(); - this.updateHighestPlUser(); - this.updatePopulationMap(); - this.updateServerCandidates(); + this.fullUpdate(); } - start() { + public start() { this.load(); - this.room.client.on(RoomMemberEvent.Membership, this.onMembership); - this.room.currentState.on(RoomStateEvent.Events, this.onRoomState); + this.room.client.on(RoomStateEvent.Update, this.onRoomStateUpdate); this.started = true; } - stop() { - this.room.client.removeListener(RoomMemberEvent.Membership, this.onMembership); - this.room.currentState.removeListener(RoomStateEvent.Events, this.onRoomState); + public stop() { + this.room.client.removeListener(RoomStateEvent.Update, this.onRoomStateUpdate); this.started = false; } - get serverCandidates() { + public get serverCandidates() { return this._serverCandidates; } - isStarted() { + public isStarted() { return this.started; } - forEvent(eventId: string): string { + public forEvent(eventId: string): string { return getPermalinkConstructor().forEvent(this.roomId, eventId, this._serverCandidates); } - forShareableRoom(): string { + public forShareableRoom(): string { if (this.room) { // Prefer to use canonical alias for permalink if possible const alias = this.room.getCanonicalAlias(); @@ -167,43 +152,28 @@ export class RoomPermalinkCreator { return getPermalinkConstructor().forRoom(this.roomId, this._serverCandidates); } - forRoom(): string { + public forRoom(): string { return getPermalinkConstructor().forRoom(this.roomId, this._serverCandidates); } - private onRoomState = (event: MatrixEvent) => { - switch (event.getType()) { - case EventType.RoomServerAcl: - this.updateAllowedServers(); - this.updateHighestPlUser(); - this.updatePopulationMap(); - this.updateServerCandidates(); - return; - case EventType.RoomPowerLevels: - this.updateHighestPlUser(); - this.updateServerCandidates(); - return; - } + private onRoomStateUpdate = () => { + this.fullUpdate(); }; - private onMembership = (evt: MatrixEvent, member: RoomMember, oldMembership: string) => { - if (member.roomId !== this.room.roomId) return; - - const userId = member.userId; - const membership = member.membership; - const serverName = getServerName(userId); - const hasJoined = oldMembership !== "join" && membership === "join"; - const hasLeft = oldMembership === "join" && membership !== "join"; - - if (hasLeft) { - this.populationMap[serverName]--; - } else if (hasJoined) { - this.populationMap[serverName]++; - } - + private fullUpdate() { + // This updates the internal state of this object from the room state. It's broken + // down into separate functions, previously because we did some of these as incremental + // updates, but they were on member events which can be very numerous, so the incremental + // updates ended up being much slower than a full update. We now have the batch state update + // event, so we just update in full, but on each batch of updates. + // A full update takes about 120ms for me on Matrix HQ, which still feels like way too long + // to be spending worrying about how we might generate a permalink, but it's better than + // multiple seconds. + this.updateAllowedServers(); this.updateHighestPlUser(); + this.updatePopulationMap(); this.updateServerCandidates(); - }; + } private updateHighestPlUser() { const plEvent = this.room.currentState.getStateEvents("m.room.power_levels", ""); diff --git a/test/utils/permalinks/Permalinks-test.js b/test/utils/permalinks/Permalinks-test.js index 9d311d5f429..e05cf252e4e 100644 --- a/test/utils/permalinks/Permalinks-test.js +++ b/test/utils/permalinks/Permalinks-test.js @@ -122,14 +122,14 @@ describe('Permalinks', function() { }, member95, ]); - const creator = new RoomPermalinkCreator(room, null, false); + const creator = new RoomPermalinkCreator(room, null); creator.load(); expect(creator._serverCandidates[0]).toBe("pl_95"); member95.membership = "left"; - creator.onMembership({}, member95, "join"); + creator.onRoomStateUpdate(); expect(creator._serverCandidates[0]).toBe("pl_75"); member95.membership = "join"; - creator.onMembership({}, member95, "left"); + creator.onRoomStateUpdate(); expect(creator._serverCandidates[0]).toBe("pl_95"); }); From 818de2bfa6a4d0511a0d0344ea4e0dc9688bf5b7 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 25 Feb 2022 17:34:21 +0000 Subject: [PATCH 2/2] Just bind to the currentstate state updates --- src/utils/permalinks/Permalinks.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/permalinks/Permalinks.ts b/src/utils/permalinks/Permalinks.ts index b8349f5fbf8..ebd789267b6 100644 --- a/src/utils/permalinks/Permalinks.ts +++ b/src/utils/permalinks/Permalinks.ts @@ -120,12 +120,12 @@ export class RoomPermalinkCreator { public start() { this.load(); - this.room.client.on(RoomStateEvent.Update, this.onRoomStateUpdate); + this.room.currentState.on(RoomStateEvent.Update, this.onRoomStateUpdate); this.started = true; } public stop() { - this.room.client.removeListener(RoomStateEvent.Update, this.onRoomStateUpdate); + this.room.currentState.removeListener(RoomStateEvent.Update, this.onRoomStateUpdate); this.started = false; }