Skip to content

Commit aee0732

Browse files
authored
do a single pass in maxByFirstDiffering (#1824)
This cuts the runtime for hat allocation in my tests by about 20%. No functional changes. I have not added any tests, but I have confirmed that there are no functional changes using my not-yet-submitted hat golden tests. Part of the reason that the hat golden tests are not yet ready for review is that they are way too slow. ## Checklist - [/] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [/] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [/] I have not broken the cheatsheet
1 parent 21b5868 commit aee0732

File tree

2 files changed

+75
-5
lines changed

2 files changed

+75
-5
lines changed
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import * as assert from "assert";
2+
import { maxByAllowingTies } from "./maxByFirstDiffering";
3+
4+
// known good but slow
5+
function goldenMaxByAllowingTies<T>(arr: T[], fn: (item: T) => number): T[] {
6+
const max = Math.max(...arr.map(fn));
7+
return arr.filter((item) => fn(item) === max);
8+
}
9+
10+
suite("maxByFirstDiffering", () => {
11+
test("maxByAllowingTies", () => {
12+
const testCases: number[][] = [
13+
[],
14+
[0],
15+
[1],
16+
[-Infinity],
17+
[+Infinity],
18+
[0, 0],
19+
[0, 1],
20+
[1, 0],
21+
[-Infinity, -Infinity],
22+
[-Infinity, +Infinity],
23+
[+Infinity, -Infinity],
24+
[+Infinity, 0],
25+
[0, +Infinity],
26+
[-Infinity, 0],
27+
[0, -Infinity],
28+
[0, 1, 0, 1],
29+
[1, 0, 1, 0],
30+
[0, 1, 1, 0],
31+
[1, 0, 0, 1],
32+
];
33+
34+
testCases.forEach((testCase) => {
35+
const actual = maxByAllowingTies(testCase, (x) => x);
36+
const expected = goldenMaxByAllowingTies(testCase, (x) => x);
37+
assert.deepStrictEqual(actual, expected);
38+
});
39+
});
40+
});

packages/cursorless-engine/src/util/allocateHats/maxByFirstDiffering.ts

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,45 @@ export function maxByFirstDiffering<T>(
2424
return undefined;
2525
}
2626
let remainingValues = arr;
27-
2827
for (const fn of fns) {
2928
if (remainingValues.length === 1) {
3029
return remainingValues[0];
3130
}
32-
33-
const max = Math.max(...remainingValues.map(fn));
34-
remainingValues = remainingValues.filter((item) => fn(item) === max);
31+
remainingValues = maxByAllowingTies(remainingValues, fn);
3532
}
36-
3733
return remainingValues[0];
3834
}
35+
36+
/**
37+
* Given an array of items and a function that returns a number for each item,
38+
* return all items that share the maximum value according to that function.
39+
* @param arr The array to find the max values of
40+
* @param fn A function that returns a number for each item in the array
41+
* @returns All items in the array that share the maximum value
42+
**/
43+
export function maxByAllowingTies<T>(arr: T[], fn: (item: T) => number): T[] {
44+
// This is equivalent to, but faster than:
45+
//
46+
// const max = Math.max(...arr.map(fn));
47+
// return arr.filter((item) => fn(item) === max);
48+
//
49+
// It does only a single pass through the array, and allocates no
50+
// intermediate arrays (in the common case).
51+
52+
// Accumulate all items with the single highest value,
53+
// resetting whenever we find a new highest value.
54+
let best: number = -Infinity;
55+
const keep: T[] = [];
56+
for (const item of arr) {
57+
const value = fn(item);
58+
if (value < best) {
59+
continue;
60+
}
61+
if (value > best) {
62+
best = value;
63+
keep.length = 0;
64+
}
65+
keep.push(item);
66+
}
67+
return keep;
68+
}

0 commit comments

Comments
 (0)