-
-
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 24 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,35 @@ | ||
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[]> = {}; | ||
|
||
clear() { | ||
this.documentUri = ""; | ||
this.documentVersion = -1; | ||
this.captures = {}; | ||
} | ||
|
||
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,11 @@ 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 |
||
*/ | ||
clearCache(): void; | ||
|
||
/** | ||
* @deprecated Only for use in legacy containing scope stage | ||
*/ | ||
|
@@ -155,6 +160,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,49 @@ | ||
import type { Range } from "@cursorless/common"; | ||
|
||
/** | ||
* An iterator that allows for efficient lookup of ranges that contain a search item. | ||
* The items must be sorted in document order. | ||
*/ | ||
export class RangeLookupList<T extends { range: Range }> { | ||
AndreasArvidsson marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
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; | ||
} | ||
} |
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. A few unit tests would be nice here. It's complex enough, and would function as a form of documentation as well 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. Added a few tests |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
import type { Range } from "@cursorless/common"; | ||
import { RangeLookupList } from "./RangeLookupList"; | ||
|
||
/** | ||
* A tree of ranges that allows for efficient lookup of ranges that contain a search item. | ||
* The items must be sorted in document order. | ||
*/ | ||
export class RangeLookupTree<T extends { range: Range }> { | ||
AndreasArvidsson marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
private children: RangeLookupList<RangeLookupTreeNode<T>>; | ||
|
||
/** | ||
* @param items The items to search in. Must be sorted in document order. | ||
*/ | ||
constructor(items: T[]) { | ||
this.children = createNodes(items); | ||
} | ||
|
||
getSmallLestContaining(separator: Range): T | undefined { | ||
return this.children | ||
.getContaining(separator) | ||
?.getSmallLestContaining(separator); | ||
} | ||
} | ||
AndreasArvidsson marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
function createNodes<T extends { range: Range }>( | ||
items: T[], | ||
): RangeLookupList<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 RangeLookupList(results); | ||
} | ||
|
||
class RangeLookupTreeNode<T extends { range: Range }> { | ||
public children: RangeLookupList<RangeLookupTreeNode<T>>; | ||
|
||
constructor(private item: T) { | ||
this.children = new RangeLookupList([]); | ||
} | ||
|
||
get range(): Range { | ||
return this.item.range; | ||
} | ||
|
||
getSmallLestContaining(range: Range): T { | ||
const child = this.children | ||
.getContaining(range) | ||
?.getSmallLestContaining(range); | ||
|
||
return child ?? this.item; | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.