Skip to content

Commit c5824a1

Browse files
AndreasArvidssonpokey
andauthoredJan 7, 2025··
Fix surrounding pair performance (#2700)
With a 10k line json. `"take pair"` went from about 12 seconds to 200 ms on my computer ## Checklist - [x] 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 --------- Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
1 parent c2d3766 commit c5824a1

File tree

16 files changed

+352
-46
lines changed

16 files changed

+352
-46
lines changed
 
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
languageId: typescript
2+
command:
3+
version: 7
4+
spokenForm: change pair
5+
action:
6+
name: clearAndSetSelection
7+
target:
8+
type: primitive
9+
modifiers:
10+
- type: containingScope
11+
scopeType: {type: surroundingPair, delimiter: any}
12+
usePrePhraseSnapshot: true
13+
initialState:
14+
documentContents: "[1, ']', 2]"
15+
selections:
16+
- anchor: {line: 0, character: 1}
17+
active: {line: 0, character: 1}
18+
marks: {}
19+
finalState:
20+
documentContents: ""
21+
selections:
22+
- anchor: {line: 0, character: 0}
23+
active: {line: 0, character: 0}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
languageId: typescript
2+
command:
3+
version: 7
4+
spokenForm: change pair
5+
action:
6+
name: clearAndSetSelection
7+
target:
8+
type: primitive
9+
modifiers:
10+
- type: containingScope
11+
scopeType: {type: surroundingPair, delimiter: any}
12+
usePrePhraseSnapshot: true
13+
initialState:
14+
documentContents: "`[1, ${']'}, 3]`"
15+
selections:
16+
- anchor: {line: 0, character: 1}
17+
active: {line: 0, character: 1}
18+
marks: {}
19+
finalState:
20+
documentContents: "``"
21+
selections:
22+
- anchor: {line: 0, character: 1}
23+
active: {line: 0, character: 1}

‎packages/common/src/index.ts

+1
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ export * from "./types/Selection";
7676
export * from "./types/snippet.types";
7777
export * from "./types/SpokenForm";
7878
export * from "./types/SpokenFormType";
79+
export * from "./types/StringRecord";
7980
export * from "./types/TalonSpokenForms";
8081
export * from "./types/TestCaseFixture";
8182
export * from "./types/TestHelpers";

‎packages/common/src/util/regex.ts

+9-5
Original file line numberDiff line numberDiff line change
@@ -30,16 +30,20 @@ function makeCache<T, U>(func: (arg: T) => U) {
3030
export const rightAnchored = makeCache(_rightAnchored);
3131
export const leftAnchored = makeCache(_leftAnchored);
3232

33+
export function matchAllIterator(text: string, regex: RegExp) {
34+
// Reset the regex to start at the beginning of string, in case the regex has
35+
// been used before.
36+
// See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/exec#finding_successive_matches
37+
regex.lastIndex = 0;
38+
return text.matchAll(regex);
39+
}
40+
3341
export function matchAll<T>(
3442
text: string,
3543
regex: RegExp,
3644
mapfn: (v: RegExpMatchArray, k: number) => T,
3745
) {
38-
// Reset the regex to start at the beginning of string, in case the regex has
39-
// been used before.
40-
// See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/exec#finding_successive_matches
41-
regex.lastIndex = 0;
42-
return Array.from(text.matchAll(regex), mapfn);
46+
return Array.from(matchAllIterator(text, regex), mapfn);
4347
}
4448

4549
export function testRegex(regex: RegExp, text: string): boolean {

‎packages/cursorless-engine/src/actions/ExecuteCommand.ts

+1
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import type { ActionReturnValue } from "./actions.types";
1414
*/
1515
export default class ExecuteCommand {
1616
private callbackAction: CallbackAction;
17+
1718
constructor(rangeUpdater: RangeUpdater) {
1819
this.callbackAction = new CallbackAction(rangeUpdater);
1920
this.run = this.run.bind(this);

‎packages/cursorless-engine/src/disabledComponents/DisabledLanguageDefinitions.ts

+4
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ export class DisabledLanguageDefinitions implements LanguageDefinitions {
1616
return undefined;
1717
}
1818

19+
clearCache(): void {
20+
// Do nothing
21+
}
22+
1923
getNodeAtLocation(
2024
_document: TextDocument,
2125
_range: Range,

‎packages/cursorless-engine/src/languages/LanguageDefinition.ts

+35-5
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type {
33
ScopeType,
44
SimpleScopeType,
55
SimpleScopeTypeType,
6+
StringRecord,
67
TreeSitter,
78
} from "@cursorless/common";
89
import {
@@ -12,6 +13,7 @@ import {
1213
type TextDocument,
1314
} from "@cursorless/common";
1415
import { TreeSitterScopeHandler } from "../processTargets/modifiers/scopeHandlers";
16+
import { LanguageDefinitionCache } from "./LanguageDefinitionCache";
1517
import { TreeSitterQuery } from "./TreeSitterQuery";
1618
import type { QueryCapture } from "./TreeSitterQuery/QueryCapture";
1719
import { validateQueryCaptures } from "./TreeSitterQuery/validateQueryCaptures";
@@ -21,14 +23,18 @@ import { validateQueryCaptures } from "./TreeSitterQuery/validateQueryCaptures";
2123
* tree-sitter query used to extract scopes for the given language
2224
*/
2325
export class LanguageDefinition {
26+
private cache: LanguageDefinitionCache;
27+
2428
private constructor(
2529
/**
2630
* The tree-sitter query used to extract scopes for the given language.
2731
* Note that this query contains patterns for all scope types that the
2832
* language supports using new-style tree-sitter queries
2933
*/
3034
private query: TreeSitterQuery,
31-
) {}
35+
) {
36+
this.cache = new LanguageDefinitionCache();
37+
}
3238

3339
/**
3440
* Construct a language definition for the given language id, if the language
@@ -93,10 +99,34 @@ export class LanguageDefinition {
9399
document: TextDocument,
94100
captureName: SimpleScopeTypeType,
95101
): QueryCapture[] {
96-
return this.query
97-
.matches(document)
98-
.map((match) => match.captures.find(({ name }) => name === captureName))
99-
.filter((capture) => capture != null);
102+
if (!this.cache.isValid(document)) {
103+
this.cache.update(document, this.getCapturesMap(document));
104+
}
105+
106+
return this.cache.get(captureName);
107+
}
108+
109+
clearCache(): void {
110+
this.cache = new LanguageDefinitionCache();
111+
}
112+
113+
/**
114+
* This is a low level function that returns a map of all captures in the document.
115+
*/
116+
private getCapturesMap(document: TextDocument): StringRecord<QueryCapture[]> {
117+
const matches = this.query.matches(document);
118+
const result: StringRecord<QueryCapture[]> = {};
119+
120+
for (const match of matches) {
121+
for (const capture of match.captures) {
122+
if (result[capture.name] == null) {
123+
result[capture.name] = [];
124+
}
125+
result[capture.name]!.push(capture);
126+
}
127+
}
128+
129+
return result;
100130
}
101131
}
102132

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import type {
2+
SimpleScopeTypeType,
3+
StringRecord,
4+
TextDocument,
5+
} from "@cursorless/common";
6+
import type { QueryCapture } from "./TreeSitterQuery/QueryCapture";
7+
8+
export class LanguageDefinitionCache {
9+
private documentUri: string = "";
10+
private documentVersion: number = -1;
11+
private captures: StringRecord<QueryCapture[]> = {};
12+
13+
isValid(document: TextDocument) {
14+
return (
15+
this.documentUri === document.uri.toString() &&
16+
this.documentVersion === document.version
17+
);
18+
}
19+
20+
update(document: TextDocument, captures: StringRecord<QueryCapture[]>) {
21+
this.documentUri = document.uri.toString();
22+
this.documentVersion = document.version;
23+
this.captures = captures;
24+
}
25+
26+
get(captureName: SimpleScopeTypeType): QueryCapture[] {
27+
return this.captures[captureName] ?? [];
28+
}
29+
}

‎packages/cursorless-engine/src/languages/LanguageDefinitions.ts

+19
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,17 @@ export interface LanguageDefinitions {
3232
*/
3333
get(languageId: string): LanguageDefinition | undefined;
3434

35+
/**
36+
* Clear the cache of all language definitions. This is run at the start of a command.
37+
* This isn't strict necessary for normal user operations since whenever the user
38+
* makes a change to the document the document version is updated. When
39+
* running our test though we keep closing and reopening an untitled document.
40+
* That test document will have the same uri and version unfortunately. Also
41+
* to be completely sure there isn't some extension doing similar trickery
42+
* it's just good hygiene to clear the cache before every command.
43+
*/
44+
clearCache(): void;
45+
3546
/**
3647
* @deprecated Only for use in legacy containing scope stage
3748
*/
@@ -155,6 +166,14 @@ export class LanguageDefinitionsImpl
155166
return definition === LANGUAGE_UNDEFINED ? undefined : definition;
156167
}
157168

169+
clearCache(): void {
170+
for (const definition of this.languageDefinitions.values()) {
171+
if (definition !== LANGUAGE_UNDEFINED) {
172+
definition.clearCache();
173+
}
174+
}
175+
}
176+
158177
public getNodeAtLocation(document: TextDocument, range: Range): SyntaxNode {
159178
return this.treeSitter.getNodeAtLocation(document, range);
160179
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import { Range } from "@cursorless/common";
2+
import { OneWayNestedRangeFinder } from "./OneWayNestedRangeFinder";
3+
import assert from "assert";
4+
5+
const items = [
6+
{ range: new Range(0, 0, 0, 5) },
7+
{ range: new Range(0, 1, 0, 4) },
8+
{ range: new Range(0, 2, 0, 3) },
9+
{ range: new Range(0, 6, 0, 8) },
10+
];
11+
12+
suite("OneWayNestedRangeFinder", () => {
13+
test("getSmallestContaining 1", () => {
14+
const finder = new OneWayNestedRangeFinder(items);
15+
const actual = finder.getSmallestContaining(new Range(0, 2, 0, 2));
16+
assert.equal(actual?.range.toString(), new Range(0, 2, 0, 3).toString());
17+
});
18+
19+
test("getSmallestContaining 2", () => {
20+
const finder = new OneWayNestedRangeFinder(items);
21+
const actual = finder.getSmallestContaining(new Range(0, 7, 0, 7));
22+
assert.equal(actual?.range.toString(), new Range(0, 6, 0, 8).toString());
23+
});
24+
25+
test("getSmallestContaining 3", () => {
26+
const finder = new OneWayNestedRangeFinder(items);
27+
const actual = finder.getSmallestContaining(new Range(0, 0, 0, 0));
28+
assert.equal(actual?.range.toString(), new Range(0, 0, 0, 5).toString());
29+
});
30+
});
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import type { Range } from "@cursorless/common";
2+
import { OneWayRangeFinder } from "./OneWayRangeFinder";
3+
4+
/**
5+
* Given a list of ranges (the haystack), allows the client to search for smallest range containing a range (the needle).
6+
* Has the following requirements:
7+
* - the haystack must be sorted in document order
8+
* - **the needles must be in document order as well**. This enables us to avoid backtracking as you search for a sequence of items.
9+
* - the haystack entries **may** be nested, but one haystack entry cannot partially contain another
10+
*/
11+
export class OneWayNestedRangeFinder<T extends { range: Range }> {
12+
private children: OneWayRangeFinder<RangeLookupTreeNode<T>>;
13+
14+
/**
15+
* @param items The items to search in. Must be sorted in document order.
16+
*/
17+
constructor(items: T[]) {
18+
this.children = createNodes(items);
19+
}
20+
21+
getSmallestContaining(separator: Range): T | undefined {
22+
return this.children
23+
.getContaining(separator)
24+
?.getSmallestContaining(separator);
25+
}
26+
}
27+
28+
function createNodes<T extends { range: Range }>(
29+
items: T[],
30+
): OneWayRangeFinder<RangeLookupTreeNode<T>> {
31+
const results: RangeLookupTreeNode<T>[] = [];
32+
const parents: RangeLookupTreeNode<T>[] = [];
33+
34+
for (const item of items) {
35+
const node = new RangeLookupTreeNode(item);
36+
37+
while (
38+
parents.length > 0 &&
39+
!parents[parents.length - 1].range.contains(item.range)
40+
) {
41+
parents.pop();
42+
}
43+
44+
const parent = parents[parents.length - 1];
45+
46+
if (parent != null) {
47+
parent.children.add(node);
48+
} else {
49+
results.push(node);
50+
}
51+
52+
parents.push(node);
53+
}
54+
55+
return new OneWayRangeFinder(results);
56+
}
57+
58+
class RangeLookupTreeNode<T extends { range: Range }> {
59+
public children: OneWayRangeFinder<RangeLookupTreeNode<T>>;
60+
61+
constructor(private item: T) {
62+
this.children = new OneWayRangeFinder([]);
63+
}
64+
65+
get range(): Range {
66+
return this.item.range;
67+
}
68+
69+
getSmallestContaining(range: Range): T {
70+
const child = this.children
71+
.getContaining(range)
72+
?.getSmallestContaining(range);
73+
74+
return child ?? this.item;
75+
}
76+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
import type { Range } from "@cursorless/common";
2+
3+
/**
4+
* Given a list of ranges (the haystack), allows the client to search for a sequence of ranges (the needles).
5+
* Has the following requirements:
6+
* - the haystack must be sorted in document order
7+
* - **the needles must be in document order as well**. This enables us to avoid backtracking as you search for a sequence of items.
8+
* - the haystack entries must not overlap. Adjacent is fine
9+
*/
10+
export class OneWayRangeFinder<T extends { range: Range }> {
11+
private index = 0;
12+
13+
/**
14+
* @param items The items to search in. Must be sorted in document order.
15+
*/
16+
constructor(private items: T[]) {}
17+
18+
add(item: T) {
19+
this.items.push(item);
20+
}
21+
22+
contains(searchItem: Range): boolean {
23+
return this.advance(searchItem);
24+
}
25+
26+
getContaining(searchItem: Range): T | undefined {
27+
if (this.advance(searchItem)) {
28+
return this.items[this.index];
29+
}
30+
31+
return undefined;
32+
}
33+
34+
private advance(searchItem: Range): boolean {
35+
while (this.index < this.items.length) {
36+
const range = this.items[this.index].range;
37+
38+
if (range.contains(searchItem)) {
39+
return true;
40+
}
41+
42+
// Search item is before the range. Since the ranges are sorted, we can stop here.
43+
if (searchItem.end.isBeforeOrEqual(range.start)) {
44+
return false;
45+
}
46+
47+
this.index++;
48+
}
49+
50+
return false;
51+
}
52+
}

0 commit comments

Comments
 (0)
Please sign in to comment.