Fix surrounding pair performance #2700
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
1 task
pokey
approved these changes
Jan 7, 2025
...ne/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/RangeLookupList.ts
Outdated
Show resolved
Hide resolved
| @@ -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.
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.
Comment added. The scope visualizes the works fine.
There was a problem hiding this comment.
I guess we blow away the language def every time it hot reloads so that's why it works?
...ne/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/RangeLookupTree.ts
Outdated
Show resolved
Hide resolved
...ne/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/RangeLookupTree.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
Added a few tests
## 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
…andlers/SurroundingPairScopeHandler/RangeLookupList.ts Co-authored-by: Pokey Rule <[email protected]>
…andlers/SurroundingPairScopeHandler/RangeLookupTree.ts Co-authored-by: Pokey Rule <[email protected]>
…into srPerformance
…andlers/SurroundingPairScopeHandler/RangeLookupTree.ts Co-authored-by: Pokey Rule <[email protected]>
|
Feel free to merge when you're happy |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 7, 2025
The tests should pass once #2700 is merged This is the result from running it locally on my desktop. ``` Performance: 10202 lines JSON Remove token 24 ms √ Remove token (108ms) Select character 14 ms √ Select character (122ms) Select word 10 ms √ Select word (113ms) Select token 8 ms √ Select token (110ms) Select identifier 7 ms √ Select identifier (115ms) Select line 40 ms √ Select line (141ms) Select sentence 25 ms √ Select sentence (126ms) Select paragraph 5 ms √ Select paragraph (104ms) Select document 8 ms √ Select document (115ms) Select nonWhitespaceSequence 8 ms √ Select nonWhitespaceSequence (96ms) Select string 247 ms √ Select string (353ms) Select map 167 ms √ Select map (248ms) Select collectionKey 135 ms √ Select collectionKey (223ms) Select value 142 ms √ Select value (222ms) Select boundedParagraph 11822 ms √ Select boundedParagraph (11906ms) Select boundedNonWhitespaceSequence 13749 ms √ Select boundedNonWhitespaceSequence (13837ms) Select surroundingPair.any 20893 ms √ Select surroundingPair.any (21039ms) Select surroundingPair.curlyBrackets 365 ms √ Select surroundingPair.curlyBrackets (483ms) 18 passing (50s) ``` ## 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
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 9, 2025
Uses the cache added in #2700 to also optimize all tree sitter queries, not just surrounding pairs. ## 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 --------- Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With a 10k line json.
"take pair"went from about 12 seconds to 200 ms on my computer. Implements a cache and also eliminates nested loops and replaces them with a lookup tree.Checklist