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

CyrusNajmabadi · 2025-02-07T21:31:33Z

Followup to #77111

CyrusNajmabadi · 2025-02-07T21:36:05Z

src/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor_Helpers.cs

@@ -127,84 +128,6 @@ static string GetLanguageSpecificId(string? language, string noLanguageId, strin
                language: language);
        }

-        public static async Task<CompilationWithAnalyzersPair?> CreateCompilationWithAnalyzersAsync(


moved to local function in single place it is needed.

i also lifted the 'compilation == null' check below out of it. but it is otherwise unchanged.

CyrusNajmabadi · 2025-02-07T21:36:45Z

...r/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.CompilationManager.cs

+    /// passed along with the project.  As such, we might not be able to use a prior cached value if the set of state
+    /// sets changes.  In that case, a new instance will be created and will be cached for the next caller.
+    /// </summary>
+    private static readonly ConditionalWeakTable<Project, CompilationWithAnalyzersPair?> s_projectToCompilationWithAnalyzers = new();


changed from WeakRef to CWT. that way we can reuse when asking about other projects as well. and, as long as the project is alive we keep the CompPair (instead of dumping on first GC).

CyrusNajmabadi · 2025-02-07T21:37:16Z

...r/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.CompilationManager.cs

    {
-        private static Task<CompilationWithAnalyzersPair?> CreateCompilationWithAnalyzersAsync(Project project, ImmutableArray<StateSet> stateSets, bool crashOnAnalyzerException, CancellationToken cancellationToken)


i wanted to hide this method. no one should just make the CompPair. They shoudl go through the helper that returns if we have a cached one in the CWT.

CyrusNajmabadi · 2025-02-07T21:39:30Z

...V2/DiagnosticIncrementalAnalyzer_GetDiagnosticsForSpan.ProjectAndCompilationWithAnalyzers.cs

-        private sealed class ProjectAndCompilationWithAnalyzers
-        {
-            public Project Project { get; }
-            public CompilationWithAnalyzersPair? CompilationWithAnalyzers { get; }


instead of a weakref to one of these, we have a CWT from a Project to the last CompPair we computed for it.

CyrusNajmabadi · 2025-02-07T21:40:36Z

...rotocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnosticsForSpan.cs

-                {
-                    if (projectAndCompilationWithAnalyzers.CompilationWithAnalyzers == null)
-                    {
-                        return null;


note: i'm virtually certain this is a bug. that's because a prior call with a DIFFERENT set of 'state sets' could have caused this to be computed and cached as 'null', while THIS set of 'state sets' should have produced a legal CompPair. But because of this check we would bail out. I've fixed this in the new code.

CyrusNajmabadi · 2025-02-07T21:41:30Z

...erver/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnostics.cs

+                        return box.Value.diagnosticAnalysisResults;
+
+                    // Otherwise, just compute for the state sets we care about.
+                    var compilation = await GetOrCreateCompilationWithAnalyzersAsync(project, stateSets, Owner.AnalyzerService.CrashOnAnalyzerException, cancellationToken).ConfigureAwait(false);


calls GetOrCreateCompilationWithAnalyzersAsync instead of CreateCompilationXXX. We want this to reuse a cached value if applicable.

CyrusNajmabadi · 2025-02-07T21:41:38Z

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

+                    static (stateSet, arg) => arg.self.IsCandidateForFullSolutionAnalysis(stateSet.Analyzer, stateSet.IsHostAnalyzer, arg.project),
+                    (self: this, project));
+
+                var compilationWithAnalyzers = await GetOrCreateCompilationWithAnalyzersAsync(


calls GetOrCreateCompilationWithAnalyzersAsync instead of CreateCompilationXXX. We want this to reuse a cached value if applicable.

CyrusNajmabadi · 2025-02-08T18:50:50Z

...r/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.CompilationManager.cs

+        CancellationToken cancellationToken)
+    {
+        if (!project.SupportsCompilation)
+            return null;


lifted this up from CreateCompilationWithAnalyzersAsync

CyrusNajmabadi · 2025-02-08T18:51:58Z

...r/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.CompilationManager.cs

+        static bool HasAllAnalyzers(ImmutableArray<StateSet> stateSets, CompilationWithAnalyzersPair? compilationWithAnalyzers)
+        {
+            if (compilationWithAnalyzers is null)
+                return false;


this check is new, but is also a bug fix. we might have stored 'null' before because we decided to not produce a compilation-pair for a particular set of state-sets. taht doesn't mean we should fail from trying to produce a compilation with our current set.

CyrusNajmabadi · 2025-02-08T18:56:39Z

...r/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.CompilationManager.cs

+        // Make sure the cached pair was computed with at least the same state sets we're asking about.  if not,
+        // recompute and cache with the new state sets.
+        if (!s_projectToCompilationWithAnalyzers.TryGetValue(project, out var tupleBox) ||
+            !stateSets.IsSubsetOf(tupleBox.Value.stateSets))


changed to use same subset logic used elsewhere in this component.

CyrusNajmabadi · 2025-02-08T18:57:32Z

...rotocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnosticsForSpan.cs

-                s_lastProjectAndCompilationWithAnalyzers.SetTarget(new ProjectAndCompilationWithAnalyzers(project, compilationWithAnalyzers));
-                return compilationWithAnalyzers;
-
-                static bool HasAllAnalyzers(IEnumerable<StateSet> stateSets, CompilationWithAnalyzersPair compilationWithAnalyzers)


removed this. instead of checking that the compWithAnalyzers has all the analyzers we're caring about. we just check that the state sets we're asking about are a subset of the last state sets we computed the CompWithAnalyzers for.

CyrusNajmabadi · 2025-02-08T18:58:05Z

@JoeRobich @dibarbet this is ready for review.

JoeRobich · 2025-02-08T23:47:35Z

...r/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.CompilationManager.cs

+
+            // in IDE, we always set concurrentAnalysis == false otherwise, we can get into thread starvation due to
+            // async being used with synchronous blocking concurrency.
+            var projectAnalyzerOptions = new CompilationWithAnalyzersOptions(


Would it be worthwhile to only construct the options if we knew we were going to use them?

yes. that would make sense. this logic exists from before. but i can make that change later!

Made in 3c5c686

...r/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.CompilationManager.cs

ToddGrun · 2025-02-10T15:24:23Z

...r/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.CompilationManager.cs

+        async Task<CompilationWithAnalyzersPair?> CreateCompilationWithAnalyzersAsync()
+        {
+            var projectAnalyzers = stateSets.SelectAsArray(s => !s.IsHostAnalyzer, s => s.Analyzer);
+            var hostAnalyzers = stateSets.SelectAsArray(s => s.IsHostAnalyzer, s => s.Analyzer);


Both these arrays look to just be intermediaries used only once. Could they be removed and the filtered arrays populated something like:

var filteredProjectAnalyzers = stateSets.SelectAsArray(s => !s.IsHostAnalyzer && !s.Analyzer.IsWorkspaceDiagnosticAnalyzer(), s => s.Analyzer); var filteredHostAnalyzers = stateSets.SelectAsArray(s => s.IsHostAnalyzer && !s.Analyzer.IsWorkspaceDiagnosticAnalyzer(), s => s.Analyzer);

ToddGrun · 2025-02-10T15:33:07Z

...r/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.CompilationManager.cs

+
+            // Intentionally ignore the result of this.  We still want to use the value we computed above, even if
+            // another thread interleaves and sets a different value.
+            s_projectToCompilationWithAnalyzers.GetValue(project, _ => tupleBox);


Isn't there an AddOrUpdate on CWT?

ToddGrun · 2025-02-10T15:35:09Z

...r/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.CompilationManager.cs

+    /// passed along with the project.  As such, we might not be able to use a prior cached value if the set of state
+    /// sets changes.  In that case, a new instance will be created and will be cached for the next caller.
+    /// </summary>
+    private static readonly ConditionalWeakTable<Project, StrongBox<(ImmutableArray<StateSet> stateSets, CompilationWithAnalyzersPair? compilationWithAnalyzersPair)>> s_projectToCompilationWithAnalyzers = new();


StrongBox

Out of curiosity, why StrongBox instead of just using Tuple directly?

ToddGrun · 2025-02-10T15:37:46Z

...rotocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnosticsForSpan.cs

@@ -7,6 +7,7 @@
 using System.Collections.Immutable;
 using System.Diagnostics;
 using System.Linq;
+using System.Runtime.CompilerServices;


Was this needed?

CyrusNajmabadi added 3 commits February 7, 2025 13:06

Cleanup how we map from projects to compilations

439121f

In progress

29c8e3d

Merge branch 'diagNoMemorySave' into projectCompilationMap

26b446c

dotnet-issue-labeler bot 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 2 commits February 7, 2025 13:34

move method

bb36408

Simplify with captures

488adf0

CyrusNajmabadi commented Feb 7, 2025

View reviewed changes

CyrusNajmabadi added 2 commits February 7, 2025 17:38

Merge branch 'diagNoMemorySave' into projectCompilationMap

577cc45

Merge branch 'main' into projectCompilationMap

14610b6

CyrusNajmabadi marked this pull request as ready for review February 8, 2025 18:49

CyrusNajmabadi requested a review from a team as a code owner February 8, 2025 18:49

CyrusNajmabadi requested review from JoeRobich and dibarbet February 8, 2025 18:49

CyrusNajmabadi commented Feb 8, 2025

View reviewed changes

Simplify subset logic

71df88d

CyrusNajmabadi commented Feb 8, 2025

View reviewed changes

CyrusNajmabadi mentioned this pull request Feb 8, 2025

Remove 'state set' concept for diagnostic analysis subsystem. #77121

Merged

JoeRobich approved these changes Feb 8, 2025

View reviewed changes

CyrusNajmabadi merged commit 3b7a814 into dotnet:main Feb 9, 2025
25 checks passed

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

CyrusNajmabadi deleted the projectCompilationMap branch February 9, 2025 01:33

ToddGrun reviewed Feb 10, 2025

View reviewed changes

dotnet-bot mentioned this pull request Feb 12, 2025

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

CyrusNajmabadi commented Feb 7, 2025 •

edited

Loading

CyrusNajmabadi Feb 7, 2025

CyrusNajmabadi Feb 8, 2025

CyrusNajmabadi Feb 7, 2025

CyrusNajmabadi Feb 7, 2025

CyrusNajmabadi Feb 7, 2025

CyrusNajmabadi Feb 7, 2025

CyrusNajmabadi Feb 7, 2025

CyrusNajmabadi Feb 7, 2025

CyrusNajmabadi Feb 8, 2025

CyrusNajmabadi Feb 8, 2025

CyrusNajmabadi Feb 8, 2025

CyrusNajmabadi Feb 8, 2025

CyrusNajmabadi commented Feb 8, 2025

JoeRobich Feb 8, 2025

CyrusNajmabadi Feb 9, 2025

CyrusNajmabadi Feb 9, 2025

ToddGrun Feb 10, 2025

ToddGrun Feb 10, 2025

ToddGrun Feb 10, 2025 •

edited

Loading

ToddGrun Feb 10, 2025

		{
		private static Task<CompilationWithAnalyzersPair?> CreateCompilationWithAnalyzersAsync(Project project, ImmutableArray<StateSet> stateSets, bool crashOnAnalyzerException, CancellationToken cancellationToken)

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

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

Conversation

CyrusNajmabadi commented Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 7, 2025 •

edited

Loading

ToddGrun Feb 10, 2025 •

edited

Loading