-
-
Notifications
You must be signed in to change notification settings - Fork 91
Fix surrounding pair performance #2700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2e74170
eab4aa1
71b7b6c
db385da
3dcac4f
706127e
8dd21f1
1c3f026
b4c8b62
4663d2a
2bf3105
bc417d8
d8d44ed
6474a91
9966278
f035b77
1d7b1f0
e48da99
47dbf97
e0dbc9f
694a050
640c774
57aff0e
40621ea
195f94c
46aa82e
5ecf4a1
4fefbe2
63924c2
cd7cddf
ae2804b
1a46aa8
174760f
c4873bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
languageId: typescript | ||
command: | ||
version: 7 | ||
spokenForm: change pair | ||
action: | ||
name: clearAndSetSelection | ||
target: | ||
type: primitive | ||
modifiers: | ||
- type: containingScope | ||
scopeType: {type: surroundingPair, delimiter: any} | ||
usePrePhraseSnapshot: true | ||
initialState: | ||
documentContents: "[1, ']', 2]" | ||
selections: | ||
- anchor: {line: 0, character: 1} | ||
active: {line: 0, character: 1} | ||
marks: {} | ||
finalState: | ||
documentContents: "" | ||
selections: | ||
- anchor: {line: 0, character: 0} | ||
active: {line: 0, character: 0} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
languageId: typescript | ||
command: | ||
version: 7 | ||
spokenForm: change pair | ||
action: | ||
name: clearAndSetSelection | ||
target: | ||
type: primitive | ||
modifiers: | ||
- type: containingScope | ||
scopeType: {type: surroundingPair, delimiter: any} | ||
usePrePhraseSnapshot: true | ||
initialState: | ||
documentContents: "`[1, ${']'}, 3]`" | ||
selections: | ||
- anchor: {line: 0, character: 1} | ||
active: {line: 0, character: 1} | ||
marks: {} | ||
finalState: | ||
documentContents: "``" | ||
selections: | ||
- anchor: {line: 0, character: 1} | ||
active: {line: 0, character: 1} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import type { | ||
SimpleScopeTypeType, | ||
StringRecord, | ||
TextDocument, | ||
} from "@cursorless/common"; | ||
import type { QueryCapture } from "./TreeSitterQuery/QueryCapture"; | ||
|
||
export class LanguageDefinitionCache { | ||
private documentUri: string = ""; | ||
private documentVersion: number = -1; | ||
private captures: StringRecord<QueryCapture[]> = {}; | ||
|
||
isValid(document: TextDocument) { | ||
return ( | ||
this.documentUri === document.uri.toString() && | ||
this.documentVersion === document.version | ||
); | ||
} | ||
|
||
update(document: TextDocument, captures: StringRecord<QueryCapture[]>) { | ||
this.documentUri = document.uri.toString(); | ||
this.documentVersion = document.version; | ||
this.captures = captures; | ||
} | ||
|
||
get(captureName: SimpleScopeTypeType): QueryCapture[] { | ||
return this.captures[captureName] ?? []; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,17 @@ export interface LanguageDefinitions { | |
*/ | ||
get(languageId: string): LanguageDefinition | undefined; | ||
|
||
/** | ||
* Clear the cache of all language definitions. This is run at the start of a command. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. worth saying why this is necessary. I'm also concerned that if you need to do this, then the cache is breaking things that don't happen as a result of a command, eg does hot reloading still work with the scope visualizer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment added. The scope visualizes the works fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we blow away the language def every time it hot reloads so that's why it works? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably true |
||
* This isn't strict necessary for normal user operations since whenever the user | ||
* makes a change to the document the document version is updated. When | ||
* running our test though we keep closing and reopening an untitled document. | ||
* That test document will have the same uri and version unfortunately. Also | ||
* to be completely sure there isn't some extension doing similar trickery | ||
* it's just good hygiene to clear the cache before every command. | ||
*/ | ||
clearCache(): void; | ||
|
||
/** | ||
* @deprecated Only for use in legacy containing scope stage | ||
*/ | ||
|
@@ -155,6 +166,14 @@ export class LanguageDefinitionsImpl | |
return definition === LANGUAGE_UNDEFINED ? undefined : definition; | ||
} | ||
|
||
clearCache(): void { | ||
for (const definition of this.languageDefinitions.values()) { | ||
if (definition !== LANGUAGE_UNDEFINED) { | ||
definition.clearCache(); | ||
} | ||
} | ||
} | ||
|
||
public getNodeAtLocation(document: TextDocument, range: Range): SyntaxNode { | ||
return this.treeSitter.getNodeAtLocation(document, range); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import { Range } from "@cursorless/common"; | ||
import { OneWayNestedRangeFinder } from "./OneWayNestedRangeFinder"; | ||
import assert from "assert"; | ||
|
||
const items = [ | ||
{ range: new Range(0, 0, 0, 5) }, | ||
{ range: new Range(0, 1, 0, 4) }, | ||
{ range: new Range(0, 2, 0, 3) }, | ||
{ range: new Range(0, 6, 0, 8) }, | ||
]; | ||
|
||
suite("OneWayNestedRangeFinder", () => { | ||
test("getSmallestContaining 1", () => { | ||
const finder = new OneWayNestedRangeFinder(items); | ||
const actual = finder.getSmallestContaining(new Range(0, 2, 0, 2)); | ||
assert.equal(actual?.range.toString(), new Range(0, 2, 0, 3).toString()); | ||
}); | ||
|
||
test("getSmallestContaining 2", () => { | ||
const finder = new OneWayNestedRangeFinder(items); | ||
const actual = finder.getSmallestContaining(new Range(0, 7, 0, 7)); | ||
assert.equal(actual?.range.toString(), new Range(0, 6, 0, 8).toString()); | ||
}); | ||
|
||
test("getSmallestContaining 3", () => { | ||
const finder = new OneWayNestedRangeFinder(items); | ||
const actual = finder.getSmallestContaining(new Range(0, 0, 0, 0)); | ||
assert.equal(actual?.range.toString(), new Range(0, 0, 0, 5).toString()); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
import type { Range } from "@cursorless/common"; | ||
import { OneWayRangeFinder } from "./OneWayRangeFinder"; | ||
|
||
/** | ||
* Given a list of ranges (the haystack), allows the client to search for smallest range containing a range (the needle). | ||
* Has the following requirements: | ||
* - the haystack must be sorted in document order | ||
* - **the needles must be in document order as well**. This enables us to avoid backtracking as you search for a sequence of items. | ||
* - the haystack entries **may** be nested, but one haystack entry cannot partially contain another | ||
*/ | ||
export class OneWayNestedRangeFinder<T extends { range: Range }> { | ||
private children: OneWayRangeFinder<RangeLookupTreeNode<T>>; | ||
|
||
/** | ||
* @param items The items to search in. Must be sorted in document order. | ||
*/ | ||
constructor(items: T[]) { | ||
this.children = createNodes(items); | ||
} | ||
|
||
getSmallestContaining(separator: Range): T | undefined { | ||
return this.children | ||
.getContaining(separator) | ||
?.getSmallestContaining(separator); | ||
} | ||
} | ||
|
||
function createNodes<T extends { range: Range }>( | ||
items: T[], | ||
): OneWayRangeFinder<RangeLookupTreeNode<T>> { | ||
const results: RangeLookupTreeNode<T>[] = []; | ||
const parents: RangeLookupTreeNode<T>[] = []; | ||
|
||
for (const item of items) { | ||
const node = new RangeLookupTreeNode(item); | ||
|
||
while ( | ||
parents.length > 0 && | ||
!parents[parents.length - 1].range.contains(item.range) | ||
) { | ||
parents.pop(); | ||
} | ||
|
||
const parent = parents[parents.length - 1]; | ||
|
||
if (parent != null) { | ||
parent.children.add(node); | ||
} else { | ||
results.push(node); | ||
} | ||
|
||
parents.push(node); | ||
} | ||
|
||
return new OneWayRangeFinder(results); | ||
} | ||
|
||
class RangeLookupTreeNode<T extends { range: Range }> { | ||
public children: OneWayRangeFinder<RangeLookupTreeNode<T>>; | ||
|
||
constructor(private item: T) { | ||
this.children = new OneWayRangeFinder([]); | ||
} | ||
|
||
get range(): Range { | ||
return this.item.range; | ||
} | ||
|
||
getSmallestContaining(range: Range): T { | ||
const child = this.children | ||
.getContaining(range) | ||
?.getSmallestContaining(range); | ||
|
||
return child ?? this.item; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
import type { Range } from "@cursorless/common"; | ||
|
||
/** | ||
* Given a list of ranges (the haystack), allows the client to search for a sequence of ranges (the needles). | ||
* Has the following requirements: | ||
* - the haystack must be sorted in document order | ||
* - **the needles must be in document order as well**. This enables us to avoid backtracking as you search for a sequence of items. | ||
* - the haystack entries must not overlap. Adjacent is fine | ||
*/ | ||
export class OneWayRangeFinder<T extends { range: Range }> { | ||
private index = 0; | ||
|
||
/** | ||
* @param items The items to search in. Must be sorted in document order. | ||
*/ | ||
constructor(private items: T[]) {} | ||
|
||
add(item: T) { | ||
this.items.push(item); | ||
} | ||
|
||
contains(searchItem: Range): boolean { | ||
return this.advance(searchItem); | ||
} | ||
|
||
getContaining(searchItem: Range): T | undefined { | ||
if (this.advance(searchItem)) { | ||
return this.items[this.index]; | ||
} | ||
|
||
return undefined; | ||
} | ||
|
||
private advance(searchItem: Range): boolean { | ||
while (this.index < this.items.length) { | ||
const range = this.items[this.index].range; | ||
|
||
if (range.contains(searchItem)) { | ||
return true; | ||
} | ||
|
||
// Search item is before the range. Since the ranges are sorted, we can stop here. | ||
if (searchItem.end.isBeforeOrEqual(range.start)) { | ||
return false; | ||
} | ||
|
||
this.index++; | ||
} | ||
|
||
return false; | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.