Switch query events to just send ranges instead of full query text #19823
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
Charles-Gagnon
commented
Jun 24, 2022
|
|
||
| public set uri(newUri: string) { | ||
| this.queryRunner.uri = newUri; | ||
| this.dataService.uri = newUri; | ||
| if (this.queryRunner) { |
There was a problem hiding this comment.
While I was here fixed these as well since we're not guaranteed to have them exist when this is called
Pull Request Test Coverage Report for Build 2557389573
💛 - Coveralls |
alanrenmsft
reviewed
Jun 24, 2022
| if (this._captureEnabled && queryInfo && type === 'queryStop') { | ||
| const queryText = queryInfo.queryText ?? ''; | ||
| const textDocuments = vscode.workspace.textDocuments; | ||
| const textDocument = textDocuments.find(e => e.uri.toString() === vscode.Uri.parse(document.uri).toString()); |
There was a problem hiding this comment.
so, are e.uri.toString() and document.uri different?
There was a problem hiding this comment.
Probably not, but I wanted to be absolutely sure - hence the parsing to Uri and then toString'ing that
alanrenmsft
reviewed
Jun 24, 2022
alanrenmsft
approved these changes
Jun 24, 2022
Benjin
approved these changes
Jun 24, 2022
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.
Sending over the entire query text each time has perf implications - and it turns out there's no need for this since the extension can just get the text from the range directly. The extension host keeps its own copy of the text documents locally so it doesn't have go ever go outside of the process when doing it this way. This allowed me to greatly simplify the event logic as well.
Note that there are still perf issues with executing queries for large documents (I started hitting issues in the 100MB range). But I didn't see any noticeable impact from having these events firing (even with query history enabled) compared to stable ADS.