[LSP] Report unchanged results if diagnostic data is the same as last report #75587
Conversation
dotnet-issue-labeler
bot
added
Area-Analyzers
untriaged
| { | ||
| await ComputeAndReportCurrentDiagnosticsAsync( | ||
| context, diagnosticSource, progress, newResultId, clientCapabilities, cancellationToken).ConfigureAwait(false); | ||
| ReportCurrentDiagnostics( |
There was a problem hiding this comment.
Computation moved to run inside the cache based on what parameters it determines. Makes it much easier to reason about concurrency
| /// </summary> | ||
| internal class DiagnosticsPullCache(string uniqueKey) : VersionedPullCache<(int globalStateVersion, VersionStamp? dependentVersion), (int globalStateVersion, Checksum dependentChecksum), DiagnosticsRequestState, DiagnosticData>(uniqueKey) | ||
| { | ||
| public override async Task<(int globalStateVersion, VersionStamp? dependentVersion)> ComputeCheapVersionAsync(DiagnosticsRequestState state, CancellationToken cancellationToken) |
There was a problem hiding this comment.
moved from passing functions to impls of abstract methods
| { | ||
| /// <summary> | ||
| /// Internal cache item that updates state for a particular <see cref="Workspace"/> and <see cref="ProjectOrDocumentId"/> in <see cref="VersionedPullCache{TCheapVersion, TExpensiveVersion, TState, TComputedData}"/> | ||
| /// This type ensures that the state for a particular key is never updated concurrently for the same key (but different key states can be concurrent). |
There was a problem hiding this comment.
prior to this change, there was a single semaphore around updating resultIds for the entire cache item. However now we need to also run the computation to determine if and how the cache should be updated. I did not want to block requests for other documents on computation of diagnostics for a different diagnostic. So now we have the CacheItem which gives a finer grained semaphore to an entry for a specific workspace+proj/doc.
|
LMK when you'd like a review :) |
src/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs
Show resolved
Hide resolved
| CancellationToken cancellationToken) | ||
| { | ||
| // Ensure that we only update the cache item one at a time. | ||
| // This means that the computation of new data for this item only occurs sequentially. |
There was a problem hiding this comment.
aside: have we considered computing results in parallel higher up in workspace diagnostics? we walk the diagnostic sources serially. but we could use our parallel helpers to potentially speed that up.
There was a problem hiding this comment.
Definitely potentially possible. I'll leave that to investigate in a different PR.
| public async Task<(string, ImmutableArray<TComputedData>)?> UpdateCacheItemAsync( | ||
| VersionedPullCache<TCheapVersion, TExpensiveVersion, TState, TComputedData> cache, | ||
| PreviousPullResult? previousPullResult, | ||
| bool isFullyLoaded, |
There was a problem hiding this comment.
hrmm... what is isFullyLoaded...
There was a problem hiding this comment.
this is the operation progress stuff. We always recompute until we've fully loaded the sln. I'm not 100% convinced its necessary anymore (since checksums should change as we load), but I don't want to remove it in this PR.
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/ProtocolUnitTests/Diagnostics/DiagnosticsPullCacheTests.cs
Outdated
Show resolved
Hide resolved
| writer.WriteInt32(position.Line); | ||
| writer.WriteInt32(position.Character); | ||
| } | ||
|
|
src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs
Outdated
Show resolved
Hide resolved
|
|
||
| WriteDiagnosticDataLocation(diagnosticData.DataLocation, writer); | ||
|
|
||
| foreach (var additionalLocation in diagnosticData.AdditionalLocations) |
There was a problem hiding this comment.
Oh, I didn't notice there were multiple file spans per location. Maybe these can't be easily grouped by file location.
| { | ||
| writer.WriteInt32(position.Line); | ||
| writer.WriteInt32(position.Character); | ||
| } |
Resolves #75240
This updates the behavior of the version cache to also store and check against the hash of the last reported data. This allows us to report unchanged results when the data we re-calculate ends up being exactly the same.
This saves some overhead in serialization / deserialization and processing on the client side.