-
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
Cache diagnostics produced during workspace pull, to avoid unnecessary computations #73201
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,10 +3,15 @@ | |
// See the LICENSE file in the project root for more information. | ||
|
||
using System; | ||
using System.Collections; | ||
using System.Collections.Generic; | ||
using System.Collections.Immutable; | ||
using System.Linq; | ||
using System.Runtime.CompilerServices; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; | ||
|
||
|
@@ -21,6 +26,13 @@ public static AbstractWorkspaceDocumentDiagnosticSource CreateForCodeAnalysisDia | |
private sealed class FullSolutionAnalysisDiagnosticSource(TextDocument document, Func<DiagnosticAnalyzer, bool>? shouldIncludeAnalyzer) | ||
: AbstractWorkspaceDocumentDiagnosticSource(document) | ||
{ | ||
/// <summary> | ||
/// Cached mapping between a project instance and all the diagnostics computed for it. This is used so that | ||
/// once we compute the diagnostics once for a particular project, we don't need to recompute them again as we | ||
/// walk every document within it. | ||
/// </summary> | ||
private static readonly ConditionalWeakTable<Project, AsyncLazy<IReadOnlyList<DiagnosticData>>> s_projectToDiagnostics = new(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mavasani i didn't want to do this, but caching at the IDiagnosticAnalyzerService just seems busted currently. |
||
|
||
/// <summary> | ||
/// This is a normal document source that represents live/fresh diagnostics that should supersede everything else. | ||
/// </summary> | ||
|
@@ -40,14 +52,35 @@ public override bool IsLiveSource() | |
} | ||
else | ||
{ | ||
// We call GetDiagnosticsForIdsAsync as we want to ensure we get the full set of diagnostics for this document | ||
// including those reported as a compilation end diagnostic. These are not included in document pull (uses GetDiagnosticsForSpan) due to cost. | ||
// However we can include them as a part of workspace pull when FSA is on. | ||
var documentDiagnostics = await diagnosticAnalyzerService.GetDiagnosticsForIdsAsync( | ||
Document.Project.Solution, Document.Project.Id, Document.Id, | ||
diagnosticIds: null, shouldIncludeAnalyzer, includeSuppressedDiagnostics: false, | ||
includeLocalDocumentDiagnostics: true, includeNonLocalDocumentDiagnostics: true, cancellationToken).ConfigureAwait(false); | ||
return documentDiagnostics; | ||
var projectDiagnostics = await GetProjectDiagnosticsAsync(diagnosticAnalyzerService, cancellationToken).ConfigureAwait(false); | ||
return projectDiagnostics.WhereAsArray(d => d.DocumentId == Document.Id); | ||
} | ||
} | ||
|
||
private async ValueTask<ImmutableArray<DiagnosticData>> GetProjectDiagnosticsAsync( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it possible to add a test that verifies we're not analyzing twice? Maybe add a test-only analyzer that throws if called twice? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. i acan try to add that. |
||
IDiagnosticAnalyzerService diagnosticAnalyzerService, CancellationToken cancellationToken) | ||
{ | ||
if (!s_projectToDiagnostics.TryGetValue(Document.Project, out var lazyDiagnostics)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be good to add a comment on
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed offline. It's correct that parallel access won't happen. But it's also 100% ok if it does. CWT (and the pattern we use to access it here) is concurrency safe. |
||
{ | ||
// Extracted into local to prevent captures. | ||
lazyDiagnostics = GetLazyDiagnostics(); | ||
} | ||
|
||
var result = await lazyDiagnostics.GetValueAsync(cancellationToken).ConfigureAwait(false); | ||
return (ImmutableArray<DiagnosticData>)result; | ||
|
||
AsyncLazy<IReadOnlyList<DiagnosticData>> GetLazyDiagnostics() | ||
{ | ||
return s_projectToDiagnostics.GetValue( | ||
Document.Project, | ||
_ => AsyncLazy.Create<IReadOnlyList<DiagnosticData>>( | ||
async cancellationToken => await diagnosticAnalyzerService.GetDiagnosticsForIdsAsync( | ||
Document.Project.Solution, Document.Project.Id, documentId: null, | ||
diagnosticIds: null, shouldIncludeAnalyzer, | ||
// Ensure we compute and return diagnostics for both the normal docs and the additional docs in this project. | ||
static (project, _) => [.. project.DocumentIds.Concat(project.AdditionalDocumentIds)], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mavasani i needed to be able to do this as otherwise the built-in logic of the diag-analyzer-service is just to get the diagnostics for |
||
includeSuppressedDiagnostics: false, | ||
includeLocalDocumentDiagnostics: true, includeNonLocalDocumentDiagnostics: true, cancellationToken).ConfigureAwait(false))); | ||
} | ||
} | ||
} | ||
|
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.
@mavasani here is where we hardcoded that if you didn't pass in a document, you got only the diagnostics for the normal Documents (not including additional documents).