Fix perf of multiline semantic tokens, and LF handling of same #8828
Conversation
This file contains hidden or 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
davidwengier
commented
Jun 14, 2023
| 1 0 19 razorComment 0 | ||
| 1 0 3 razorComment 0 | ||
| 0 3 1 razorCommentStar 0 | ||
| 0 1 1 razorCommentTransition 0 |
Member
Author
There was a problem hiding this comment.
Importantly, with the previous code, this test snapshot file and the above test snapshot file were different, which is the bug this is fixing. All other test snapshots are unchanged because the rest is just perf work.
davidwengier
changed the title
...Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/TagHelperSemanticRangeVisitor.cs
Show resolved
Hide resolved
...Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/TagHelperSemanticRangeVisitor.cs
Outdated
Show resolved
Hide resolved
DustinCampbell
approved these changes
Jun 15, 2023
ryzngard
approved these changes
Jun 15, 2023
| // Blank line. The less than check is important because we could have seen two newline characters, but | ||
| // be in a situation where we have an LF file with multiple blank lines, so endChar would be -1. Still | ||
| // a blank line though :) | ||
| if (endChar <= 0) |
Contributor
There was a problem hiding this comment.
❓ would it make more sense to have GetLastNonWhitespaceCharacterOffset not return a negative number? Will comment below on proposed change
Member
Author
There was a problem hiding this comment.
Good call, thank you! Couldn't see the forrest for the trees :)
| lineLength--; | ||
|
|
||
| var lineEndAbsoluteIndex = lineStartAbsoluteIndex + lineLength; | ||
| if (lineEndAbsoluteIndex == 0) |
Contributor
There was a problem hiding this comment.
Suggested change
| if (lineEndAbsoluteIndex == 0) | |
| if (lineEndAbsoluteIndex == 0 || lineLength == 0) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixes #6756
Fixes #8176
The compiler team fixed the perf of
GetContent()in #8032 but we were still being pretty naive in our usage here, and the use ofEnvironment.NewLinewas just wrong.Results show no harm without a large comment, but a big saving in allocations when there is a large comment.
Old:
New: