Skip to content

Commit

Permalink
Remove the compare function from utils (#4315)
Browse files Browse the repository at this point in the history
* Remove the compare function from utils

and change the one use of it to just intantiate a collator and use
it.

This was marked as internal module so this shouldn't be a breaking change.
Of course, react-sdk was using it.

Requires: matrix-org/matrix-react-sdk#12782

* Add simple not-a-perf-test test

* recalculate repeatedly

otherwise we aren't testing anything different

* Use fewer members as it was making the test take a bit too long
  • Loading branch information
dbkr authored Jul 17, 2024
1 parent 30a2681 commit 6f63ff1
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 12 deletions.
16 changes: 16 additions & 0 deletions spec/unit/room.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1394,6 +1394,22 @@ describe("Room", function () {
expect(name).toEqual(userB);
});
});

it("recalculates in acceptable time without heroes", function () {
for (let i = 0; i < 5000; i++) {
addMember(`@person${i}:bar`, KnownMembership.Join, { name: `Person ${i % 20} ${i % 10} ${i % 3}` });
}

// This isn't a real performance test and has plenty of headroom because github
// runners don't offer any kind of speed consistency guarantee, but this should at
// least assert that the perf doesn't suddenly become n^2.
const start = performance.now();
for (let i = 0; i < 50; i++) {
room.recalculate();
}
const duration = performance.now() - start;
expect(duration).toBeLessThan(200);

Check failure on line 1411 in spec/unit/room.spec.ts

View workflow job for this annotation

GitHub Actions / Jest [unit] (Node 22)

Room › recalculate › recalculates in acceptable time without heroes

expect(received).toBeLessThan(expected) Expected: < 200 Received: 228.7753159999993 at Object.toBeLessThan (spec/unit/room.spec.ts:1411:30)
});
});

describe("receipts", function () {
Expand Down
5 changes: 3 additions & 2 deletions src/models/room.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
} from "./event-timeline-set";
import { Direction, EventTimeline } from "./event-timeline";
import { getHttpUriForMxc } from "../content-repo";
import { compare, removeElement } from "../utils";
import { removeElement } from "../utils";
import { normalize, noUnsafeEventProps } from "../utils";
import { IEvent, IThreadBundledRelationship, MatrixEvent, MatrixEventEvent, MatrixEventHandlerMap } from "./event";
import { EventStatus } from "./event-status";
Expand Down Expand Up @@ -3451,7 +3451,8 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
return true;
});
// make sure members have stable order
otherMembers.sort((a, b) => compare(a.userId, b.userId));
const collator = new Intl.Collator();
otherMembers.sort((a, b) => collator.compare(a.userId, b.userId));
// only 5 first members, immitate summaryHeroes
otherMembers = otherMembers.slice(0, 5);
otherNames = otherMembers.map((m) => m.name);
Expand Down
10 changes: 0 additions & 10 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -649,16 +649,6 @@ export function lexicographicCompare(a: string, b: string): number {
}
}

const collator = new Intl.Collator();
/**
* Performant language-sensitive string comparison
* @param a - the first string to compare
* @param b - the second string to compare
*/
export function compare(a: string, b: string): number {
return collator.compare(a, b);
}

/**
* This function is similar to Object.assign() but it assigns recursively and
* allows you to ignore nullish values from the source
Expand Down

0 comments on commit 6f63ff1

Please sign in to comment.