Cache diagnostics produced during workspace pull, to avoid unnecessary computations #73201
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
I have validated this fixes the issue. |
…y computations Docs
CyrusNajmabadi
force-pushed
the
workspacePullCache
branch
from
75cb3ed
to
0c31332
Compare
| private async ValueTask<ImmutableArray<DiagnosticData>> GetProjectDiagnosticsAsync( | ||
| IDiagnosticAnalyzerService diagnosticAnalyzerService, CancellationToken cancellationToken) | ||
| { | ||
| if (!s_projectToDiagnostics.TryGetValue(Document.Project, out var lazyDiagnostics)) |
There was a problem hiding this comment.
Might be good to add a comment on s_projectToDiagnostics that parallel access won't happen because
- There is only one workspace diagnostic bucket that creates the
AbstractWorkspaceDocumentDiagnosticSource - the sources are called in order
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| private async ValueTask<ImmutableArray<DiagnosticData>> GetProjectDiagnosticsAsync( |
There was a problem hiding this comment.
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.
yes. i acan try to add that.
CyrusNajmabadi
force-pushed
the
workspacePullCache
branch
from
ff73993
to
3b3fbd7
Compare
|
@mavasani for eyes as well. |
| /// 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.
@mavasani i didn't want to do this, but caching at the IDiagnosticAnalyzerService just seems busted currently.
| 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.
@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 project.DocumentIds alone. I considered tweaking the core logic, but there were a fair amount of callers of GetDiagnosticsForIdsAsync and i was concerned that they might break if they started getting diags for additional-docs as well as normal docs.
| @@ -76,7 +80,7 @@ public async Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(Cancellati | |||
| return GetDiagnosticData(); | |||
| } | |||
|
|
|||
| var documentIds = DocumentId != null ? [DocumentId] : project.DocumentIds; | |||
There was a problem hiding this comment.
@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).
Fixes devdiv.visualstudio.com/DevDiv/_workitems/edit/2045347
Fixes devdiv.visualstudio.com/DevDiv/_workitems/edit/2035586
Fixes devdiv.visualstudio.com/DevDiv/_workitems/edit/2035615