Large simplification in how diagnostics are cached in the diagnostic service. #77111

CyrusNajmabadi · 2025-02-07T18:45:17Z

It may be worthwhile to review this PR commit by commit.

The general realization is this:

The diagnostic service does have a caching mechanism to save some computed state that later requests could potentially read from. HOWEVER, the only thing actually saving that computed state was the 'ForceAnalyzeProjectAsync' call, which is what is called for the 'Run Code Analysis' feature. This feature itself doesn't ever use the cached state (it returns the value from the ForceAnalyzeProjectAsync itself). If that feature was never used, the cache was totally ineffective as no other codepaths would save their results to the cache. Note that this is actually fairly fine as most other diagnostic service consumers (like LSP pull diags) cache at their level, avoiding calling back into the service if the project checksum hasn't changed.

Now, once those other calls did go into the diagnostic service, then they might read that cached data, but only if a prior ForceAnalyzeProjectAsync had happened and if their project-dependent-version timestamp exactly matched the versions when that prior ForceAnalyzeProjectAsync call was made.

Note that project-dependent-version-timestamp represents: has this project (or dependents) changed in any way. We already have a better mechanism to represent that concept. The Project Instance itself. Whenever it or deps change, you get a new instance. So the entire caching mechanism can be replaced trivially with a CWT<Project, ... cached data ...>. The two entrypoints that previous might examine the cache then just examine this CWT to see if the result was already stored there. Otherwise they compute as appropriate.

THis MASSIVELY simplifies the code in this layer, leading to huge amounts of old, complex, buggy code that could be outright eliminated. I have doc'ed the PR to help walk through the logic leading to all of this.

CyrusNajmabadi · 2025-02-07T19:27:12Z

...eServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.AnalysisData.cs

-            public bool TryGetResult(DiagnosticAnalyzer analyzer, out DiagnosticAnalysisResult result)
-                => Result.TryGetValue(analyzer, out result);
-
-            public static async Task<ProjectAnalysisData> CreateAsync(Project project, ImmutableArray<StateSet> stateSets, CancellationToken cancellationToken)


this call effectively would never return anything useful. and as such, could be removed. At that point, this type just was holding onto a ImmDic<DiagAnalyzer, DiagResult>. So the entire type could be removed, and only that ImmDict used in its stead.

CyrusNajmabadi · 2025-02-07T19:28:03Z

...eServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.AnalysisData.cs

-                foreach (var stateSet in stateSets)
-                {
-                    var state = stateSet.GetOrCreateProjectState(project.Id);
-                    var result = await state.GetAnalysisDataAsync(project, cancellationToken).ConfigureAwait(false);


later on i will show that this call would always return nothign useful here (an 'empty/default' analysis data object). LEading to the caller always having to compute anyways.

CyrusNajmabadi · 2025-02-07T19:29:33Z

...guageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.Executor.cs


-                    if (existingData.Version == version)
-                        return existingData;


this would either:

not ever succeed (in the case where ForceProjectAnalysisAsync had not run). In which case, it is fine to remove and just do the computation path below.

succeed, if it had run and no other changes to the project had been made. However, the check for that is now pulled into the caller. So these checks are pointless and this method just bcomes a helper that always computes the results.

CyrusNajmabadi · 2025-02-07T19:29:51Z

...guageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.Executor.cs

                var version = await GetDiagnosticVersionAsync(project, cancellationToken).ConfigureAwait(false);

                var ideAnalyzers = stateSets.Select(s => s.Analyzer).Where(a => a is ProjectDiagnosticAnalyzer or DocumentDiagnosticAnalyzer).ToImmutableArrayOrEmpty();

-                if (compilationWithAnalyzers != null && TryReduceAnalyzersToRun(compilationWithAnalyzers, version, existing, out var projectAnalyzersToRun, out var hostAnalyzersToRun))


TryReduceAnalyzersToRun would always return false. i'll show that below.

CyrusNajmabadi · 2025-02-07T19:30:00Z

...guageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.Executor.cs

-                            cancellationToken).ConfigureAwait(false);
-
-                    var result = await ComputeDiagnosticsAsync(compilationWithReducedAnalyzers, project, ideAnalyzers, cancellationToken).ConfigureAwait(false);
-                    return MergeExistingDiagnostics(version, existing, result);


only call of this method. So it can be removed entirely.

CyrusNajmabadi · 2025-02-07T19:32:13Z

...guageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.Executor.cs

-            projectAnalyzers = compilationWithAnalyzers.ProjectAnalyzers.WhereAsArray(
-                static (analyzer, arg) =>
-                {
-                    if (arg.existing.TryGetValue(analyzer, out var analysisResult) &&


the crux to realize here is that 'existing' was never present (since it comes from the last cached data when ForceANalyzeProjectAsync was called, and that is not called unless user explicitly runs 'run code analysis').

And, if the user does that and the project is the same, then a higher layer reuses the cached results and we don't get here now. So this would always end up returning 'true' in this lambda (and hte lambda below).WHich would cause the arrays to stay the same length in the lowest if check in this method. leading us to return 'false' for the entire method. So the caller in the method above can delete all this unnecessary code.

then a higher layer reuses the cached results and we don't get here now.

this being the new cwt with Project keys?

CyrusNajmabadi · 2025-02-07T19:32:46Z

...rver/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InMemoryStorage.cs

-{
-    private partial class DiagnosticIncrementalAnalyzer
-    {
-        private static class InMemoryStorage


all this code away. this was only used in the ForceAnalyzeProjectAsync codepath to cache its result. But now we just cache that trivially in a CWT, so this becomes entirely irrelevant.

CyrusNajmabadi · 2025-02-07T20:54:13Z

...guageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.Executor.cs

            CompilationWithAnalyzersPair? compilationWithAnalyzers, Project project, ImmutableArray<StateSet> stateSets, CancellationToken cancellationToken)
        {
            using (Logger.LogBlock(FunctionId.Diagnostics_ProjectDiagnostic, GetProjectLogMessage, project, stateSets, cancellationToken))
            {
                try
                {
-                    var version = await GetDiagnosticVersionAsync(project, cancellationToken).ConfigureAwait(false);


'versions' ended up being virtually eliminated. GetDiagnosticVersionAsync returns project.GetDependentVersionAsync which is:

The most recent version of the project, its documents and all dependent projects and documents.

So any change to eh project at all (options, info, docs, dependencies) would rev this. no need for this at all now, since we can just use the Project instnace itself in a CWT.

CyrusNajmabadi · 2025-02-07T20:54:34Z

...guageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.Executor.cs

-            var projectLoadedSuccessfully = await project.HasSuccessfullyLoadedAsync(cancellationToken).ConfigureAwait(false);
-            if (projectLoadedSuccessfully)
+            async Task<ImmutableDictionary<DiagnosticAnalyzer, DiagnosticAnalysisResult>> RemoveCompilerSemanticErrorsIfProjectNotLoadedAsync(
+                ImmutableDictionary<DiagnosticAnalyzer, DiagnosticAnalysisResult> result)


moved all these helpers to be local functions to make it much clearer that they should not be used by anything else.

dibarbet

approving, but def want to run a speedometer on this. pr val run here - https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=10975719&view=results
PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/609417

dibarbet · 2025-02-07T20:57:52Z

.../Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs

@@ -18,26 +19,26 @@ internal partial class DiagnosticAnalyzerService
 {
    private partial class DiagnosticIncrementalAnalyzer
    {
+        private readonly ConditionalWeakTable<Project, StrongBox<(ImmutableArray<StateSet> stateSets, ProjectAnalysisData projectAnalysisData)>> _projectToForceAnalysisData = new();


could doc this (e.g. why we're using a weak table, why project vs projectId, etc)

docs added .

dibarbet · 2025-02-07T21:10:56Z

...guageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.Executor.cs

-            projectAnalyzers = compilationWithAnalyzers.ProjectAnalyzers.WhereAsArray(
-                static (analyzer, arg) =>
-                {
-                    if (arg.existing.TryGetValue(analyzer, out var analysisResult) &&


then a higher layer reuses the cached results and we don't get here now.

this being the new cwt with Project keys?

dibarbet · 2025-02-07T21:21:23Z

.../Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs

+        /// <em>all</em> the analyzers for the project.  This data can then be used by <see
+        /// cref="DiagnosticGetter.ProduceDiagnosticsAsync"/> to speed up subsequent calls through the normal <see
+        /// cref="IDiagnosticAnalyzerService"/> entry points as long as the project hasn't changed at all.
+        /// </summary>


ignore my previous comment then :)

CyrusNajmabadi · 2025-02-08T17:35:19Z

No regressions reported. So merging in.

…r a particular project in the diagnostics layer. (#77113) Followup to #77111

CyrusNajmabadi added 5 commits February 7, 2025 10:05

Add cache

e661250

in progress

56b5058

Merge branch 'combineDiagTypes' into diagNoMemorySave

06f237b

in progrss

f5d7181

Remove unneeded code

f4ef9a8

ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 7, 2025

dotnet-policy-service bot added the VSCode label Feb 7, 2025

CyrusNajmabadi added 10 commits February 7, 2025 10:46

deletE

26cd9e5

rename

d34bbd7

Delete unused

84f284c

remove unneeded code from ProjectState

0e07eb2

Remove 'initial result' concept

61fd975

remove aggregated result concept

a23b304

Simplify

fb967b1

remove more unnecessary code

58d6940

Use cache if available

6619762

Remove project state concept

72ea224

CyrusNajmabadi force-pushed the diagNoMemorySave branch from aa635c6 to 72ea224 Compare February 7, 2025 19:08

CyrusNajmabadi added 4 commits February 7, 2025 11:12

Simplify type

968028d

Delete uneeded type

de1e66f

Simplify state set

20ebfb1

remove redundant type

da333ac

CyrusNajmabadi commented Feb 7, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into diagNoMemorySave

4b664b4

CyrusNajmabadi changed the title ~~WIP: Update caching for diagnostic service.~~ Massively simplify how diagnostics are cached in the diagnostic service. Feb 7, 2025

CyrusNajmabadi marked this pull request as ready for review February 7, 2025 20:39

CyrusNajmabadi requested a review from a team as a code owner February 7, 2025 20:39

CyrusNajmabadi changed the title ~~Massively simplify how diagnostics are cached in the diagnostic service.~~ Large simplification in how diagnostics are cached in the diagnostic service. Feb 7, 2025

CyrusNajmabadi added 3 commits February 7, 2025 12:44

Make local functinos

6e6d12b

Capture values

374007b

Remove version flag

a53fce1

CyrusNajmabadi commented Feb 7, 2025

View reviewed changes

CyrusNajmabadi added 2 commits February 7, 2025 13:08

Fix

8acdcc2

Update test

dc09a70

dibarbet approved these changes Feb 7, 2025

View reviewed changes

JoeRobich approved these changes Feb 7, 2025

View reviewed changes

CyrusNajmabadi mentioned this pull request Feb 7, 2025

Simplify the computation and caching of 'CompilationWithAnalyzers' for a particular project in the diagnostics layer. #77113

Merged

Add a paranoia check

0d96912

CyrusNajmabadi force-pushed the diagNoMemorySave branch from bd675fb to 0d96912 Compare February 7, 2025 21:52

CyrusNajmabadi mentioned this pull request Feb 8, 2025

Make diagnostics storage into instance state per workspace not global static state #77103

Closed

Merge remote-tracking branch 'upstream/main' into diagNoMemorySave

4b4ca0b

CyrusNajmabadi mentioned this pull request Feb 8, 2025

WIP: Break diagnostics interface into two separate interfaces #77105

Closed

CyrusNajmabadi merged commit 0fcaef9 into dotnet:main Feb 8, 2025
25 checks passed

CyrusNajmabadi deleted the diagNoMemorySave branch February 8, 2025 17:35

dotnet-policy-service bot added this to the Next milestone Feb 8, 2025

CyrusNajmabadi added a commit that referenced this pull request Feb 9, 2025

Simplify the computation and caching of 'CompilationWithAnalyzers' fo…

3b7a814

…r a particular project in the diagnostics layer. (#77113) Followup to #77111

This was referenced Feb 12, 2025

[Automated] PRs inserted in VS build main-35811.51 #77170

Closed

[Automated] PRs inserted in VS build feature.d18initial-10313.01 #77218

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35813.224 #77225

Closed

akhera99 modified the milestones: Next, 17.14 P2 Feb 25, 2025

Large simplification in how diagnostics are cached in the diagnostic service. #77111

Large simplification in how diagnostics are cached in the diagnostic service. #77111

Uh oh!

Conversation

CyrusNajmabadi commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dibarbet left a comment • edited by CyrusNajmabadi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Feb 8, 2025

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Feb 7, 2025 •

edited

Loading

dibarbet left a comment •

edited by CyrusNajmabadi

Loading