-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Reduce cost of syntactic classification by around 25% while paging down through a file (and around 50% while arrowing down). #73673
Conversation
How many of these cases are caused by us sending a change notification for something that didn't change (and the editor correctly responding by asking for the new data), and how many are cases where we didn't indicate a change but the editor still sent a request? |
.../Classification/Syntactic/SyntacticClassificationTaggerProvider.TagComputer.LastLineCache.cs
Outdated
Show resolved
Hide resolved
None. With syntactic classification we only trigger a reclassification in the following cases:
It's just the case that as we're scrolling, the editor calls into us for lines they already asked about. |
...s/Core/Classification/Syntactic/SyntacticClassificationTaggerProvider.ClassifiedLineCache.cs
Outdated
Show resolved
Hide resolved
...s/Core/Classification/Syntactic/SyntacticClassificationTaggerProvider.ClassifiedLineCache.cs
Outdated
Show resolved
Hide resolved
…sificationTaggerProvider.ClassifiedLineCache.cs
existingNode.Value.ClassifiedSpans.AddRange(classifications); | ||
} | ||
|
||
private void UpdateExistingEntryInCache( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
that would happen in the case, for example, if we did an update notification (say because parse options change). We would then generate new classifications, likely at the same span as an existing span.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated ot remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -339,6 +339,8 @@ private ImmutableArray<LSP.Diagnostic> ConvertDiagnostic(IDiagnosticSource diagn | |||
return [diagnostic]; | |||
} | |||
|
|||
return []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a bit odd
src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs
Outdated
Show resolved
Hide resolved
…actPullDiagnosticHandler.cs
Takes us from:
To
--
And from:
to:
The simple idea here is to cache more classified lines that the editor asks us about. Empirical testing showed that during page-down, we get asked for lines we were recently asked about 25% of the time, and arrowing down increased that to 85%.
By caching more (at a fixed, but larger size than the current cache), we dropped the number of times we actually have to go back to syntax by quite a bit. This helped us avoid the expensive walks of the tree, which is the limiting factor currently in large files.