From 950d6072f9a8fe7243c3930a177982944dd11e98 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 5 Feb 2025 09:40:18 -0800 Subject: [PATCH 1/2] Switch to strongly typed keys in diagnostics subsystem --- ...sticIncrementalAnalyzer.InMemoryStorage.cs | 23 ++++--------- ...gnosticIncrementalAnalyzer.ProjectState.cs | 34 ++++++++----------- .../DiagnosticAnalyzer/DiagnosticComputer.cs | 1 + 3 files changed, 23 insertions(+), 35 deletions(-) diff --git a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InMemoryStorage.cs b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InMemoryStorage.cs index ca96681acd08d..fbd4ad6e013cb 100644 --- a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InMemoryStorage.cs +++ b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InMemoryStorage.cs @@ -4,6 +4,7 @@ using System.Collections.Concurrent; using System.Collections.Immutable; +using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.Diagnostics.EngineV2 @@ -13,10 +14,10 @@ internal partial class DiagnosticIncrementalAnalyzer private static class InMemoryStorage { // the reason using nested map rather than having tuple as key is so that I dont have a gigantic map - private static readonly ConcurrentDictionary> s_map = + private static readonly ConcurrentDictionary> s_map = new(concurrencyLevel: 2, capacity: 10); - public static bool TryGetValue(DiagnosticAnalyzer analyzer, (object key, string stateKey) key, out CacheEntry entry) + public static bool TryGetValue(DiagnosticAnalyzer analyzer, (ProjectOrDocumentId key, string stateKey) key, out CacheEntry entry) { AssertKey(key); @@ -25,16 +26,16 @@ public static bool TryGetValue(DiagnosticAnalyzer analyzer, (object key, string analyzerMap.TryGetValue(key, out entry); } - public static void Cache(DiagnosticAnalyzer analyzer, (object key, string stateKey) key, CacheEntry entry) + public static void Cache(DiagnosticAnalyzer analyzer, (ProjectOrDocumentId key, string stateKey) key, CacheEntry entry) { AssertKey(key); // add new cache entry - var analyzerMap = s_map.GetOrAdd(analyzer, _ => new ConcurrentDictionary<(object key, string stateKey), CacheEntry>(concurrencyLevel: 2, capacity: 10)); + var analyzerMap = s_map.GetOrAdd(analyzer, _ => new ConcurrentDictionary<(ProjectOrDocumentId key, string stateKey), CacheEntry>(concurrencyLevel: 2, capacity: 10)); analyzerMap[key] = entry; } - public static void Remove(DiagnosticAnalyzer analyzer, (object key, string stateKey) key) + public static void Remove(DiagnosticAnalyzer analyzer, (ProjectOrDocumentId key, string stateKey) key) { AssertKey(key); // remove the entry @@ -63,16 +64,6 @@ private static void AssertKey((object key, string stateKey) key) } // in memory cache entry - private readonly struct CacheEntry - { - public readonly VersionStamp Version; - public readonly ImmutableArray Diagnostics; - - public CacheEntry(VersionStamp version, ImmutableArray diagnostics) - { - Version = version; - Diagnostics = diagnostics; - } - } + private readonly record struct CacheEntry(VersionStamp Version, ImmutableArray Diagnostics); } } diff --git a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs index 26b1624548a88..9426acc708ac8 100644 --- a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs +++ b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs @@ -7,6 +7,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Host; +using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; using Microsoft.CodeAnalysis.Options; using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.Extensions; @@ -226,12 +227,12 @@ public async ValueTask SaveToInMemoryStorageAsync(Project project, DiagnosticAna continue; } - AddToInMemoryStorage(serializerVersion, project, document, document.Id, SyntaxStateName, result.GetDocumentDiagnostics(document.Id, AnalysisKind.Syntax)); - AddToInMemoryStorage(serializerVersion, project, document, document.Id, SemanticStateName, result.GetDocumentDiagnostics(document.Id, AnalysisKind.Semantic)); - AddToInMemoryStorage(serializerVersion, project, document, document.Id, NonLocalStateName, result.GetDocumentDiagnostics(document.Id, AnalysisKind.NonLocal)); + AddToInMemoryStorage(serializerVersion, new(document.Id), SyntaxStateName, result.GetDocumentDiagnostics(document.Id, AnalysisKind.Syntax)); + AddToInMemoryStorage(serializerVersion, new(document.Id), SemanticStateName, result.GetDocumentDiagnostics(document.Id, AnalysisKind.Semantic)); + AddToInMemoryStorage(serializerVersion, new(document.Id), NonLocalStateName, result.GetDocumentDiagnostics(document.Id, AnalysisKind.NonLocal)); } - AddToInMemoryStorage(serializerVersion, project, document: null, result.ProjectId, NonLocalStateName, result.GetOtherDiagnostics()); + AddToInMemoryStorage(serializerVersion, new(result.ProjectId), NonLocalStateName, result.GetOtherDiagnostics()); } private async Task LoadInitialAnalysisDataAsync(Project project, CancellationToken cancellationToken) @@ -286,11 +287,8 @@ private async Task LoadInitialProjectAnalysisDataAsync } private void AddToInMemoryStorage( - VersionStamp serializerVersion, Project project, TextDocument? document, object key, string stateKey, ImmutableArray diagnostics) + VersionStamp serializerVersion, ProjectOrDocumentId key, string stateKey, ImmutableArray diagnostics) { - Contract.ThrowIfFalse(document == null || document.Project == project); - - // if serialization fail, hold it in the memory InMemoryStorage.Cache(_owner.Analyzer, (key, stateKey), new CacheEntry(serializerVersion, diagnostics)); } @@ -300,7 +298,7 @@ private async ValueTask TryGetDiagnosticsFromInMemoryStorageAsync(VersionS var project = document.Project; var documentId = document.Id; - var diagnostics = await GetDiagnosticsFromInMemoryStorageAsync(serializerVersion, project, document, documentId, SyntaxStateName, cancellationToken).ConfigureAwait(false); + var diagnostics = await GetDiagnosticsFromInMemoryStorageAsync(serializerVersion, new(documentId), SyntaxStateName, cancellationToken).ConfigureAwait(false); if (!diagnostics.IsDefault) { builder.AddSyntaxLocals(documentId, diagnostics); @@ -310,7 +308,7 @@ private async ValueTask TryGetDiagnosticsFromInMemoryStorageAsync(VersionS success = false; } - diagnostics = await GetDiagnosticsFromInMemoryStorageAsync(serializerVersion, project, document, documentId, SemanticStateName, cancellationToken).ConfigureAwait(false); + diagnostics = await GetDiagnosticsFromInMemoryStorageAsync(serializerVersion, new(documentId), SemanticStateName, cancellationToken).ConfigureAwait(false); if (!diagnostics.IsDefault) { builder.AddSemanticLocals(documentId, diagnostics); @@ -320,7 +318,7 @@ private async ValueTask TryGetDiagnosticsFromInMemoryStorageAsync(VersionS success = false; } - diagnostics = await GetDiagnosticsFromInMemoryStorageAsync(serializerVersion, project, document, documentId, NonLocalStateName, cancellationToken).ConfigureAwait(false); + diagnostics = await GetDiagnosticsFromInMemoryStorageAsync(serializerVersion, new(documentId), NonLocalStateName, cancellationToken).ConfigureAwait(false); if (!diagnostics.IsDefault) { builder.AddNonLocals(documentId, diagnostics); @@ -335,7 +333,7 @@ private async ValueTask TryGetDiagnosticsFromInMemoryStorageAsync(VersionS private async ValueTask TryGetProjectDiagnosticsFromInMemoryStorageAsync(VersionStamp serializerVersion, Project project, Builder builder, CancellationToken cancellationToken) { - var diagnostics = await GetDiagnosticsFromInMemoryStorageAsync(serializerVersion, project, document: null, project.Id, NonLocalStateName, cancellationToken).ConfigureAwait(false); + var diagnostics = await GetDiagnosticsFromInMemoryStorageAsync(serializerVersion, new(project.Id), NonLocalStateName, cancellationToken).ConfigureAwait(false); if (!diagnostics.IsDefault) { builder.AddOthers(diagnostics); @@ -346,10 +344,8 @@ private async ValueTask TryGetProjectDiagnosticsFromInMemoryStorageAsync(V } private ValueTask> GetDiagnosticsFromInMemoryStorageAsync( - VersionStamp serializerVersion, Project project, TextDocument? document, object key, string stateKey, CancellationToken _) + VersionStamp serializerVersion, ProjectOrDocumentId key, string stateKey, CancellationToken _) { - Contract.ThrowIfFalse(document == null || document.Project == project); - return InMemoryStorage.TryGetValue(_owner.Analyzer, (key, stateKey), out var entry) && serializerVersion == entry.Version ? new(entry.Diagnostics) : default; @@ -366,12 +362,12 @@ private void RemoveInMemoryCache(DiagnosticAnalysisResult lastResult) private void RemoveInMemoryCacheEntries(DocumentId id) { - RemoveInMemoryCacheEntry(id, SyntaxStateName); - RemoveInMemoryCacheEntry(id, SemanticStateName); - RemoveInMemoryCacheEntry(id, NonLocalStateName); + RemoveInMemoryCacheEntry(new(id), SyntaxStateName); + RemoveInMemoryCacheEntry(new(id), SemanticStateName); + RemoveInMemoryCacheEntry(new(id), NonLocalStateName); } - private void RemoveInMemoryCacheEntry(object key, string stateKey) + private void RemoveInMemoryCacheEntry(ProjectOrDocumentId key, string stateKey) { // remove in memory cache if entry exist InMemoryStorage.Remove(_owner.Analyzer, (key, stateKey)); diff --git a/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs b/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs index d8b597c517736..45f6e5bc6314f 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs @@ -576,6 +576,7 @@ private async Task CreateCompilationWithAnal { hostAnalyzerBuilder.AddRange(analyzers); } + analyzerMapBuilder.AppendAnalyzerMap(analyzers); } From 58092a9358bed3f5a1fd2859966993daff8d33d0 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 5 Feb 2025 13:21:43 -0800 Subject: [PATCH 2/2] Fix asserts --- .../DiagnosticIncrementalAnalyzer.InMemoryStorage.cs | 9 --------- .../Protocol/Handler/Diagnostics/ProjectOrDocumentId.cs | 4 +--- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InMemoryStorage.cs b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InMemoryStorage.cs index fbd4ad6e013cb..4895a9c81c5aa 100644 --- a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InMemoryStorage.cs +++ b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InMemoryStorage.cs @@ -19,8 +19,6 @@ private static class InMemoryStorage public static bool TryGetValue(DiagnosticAnalyzer analyzer, (ProjectOrDocumentId key, string stateKey) key, out CacheEntry entry) { - AssertKey(key); - entry = default; return s_map.TryGetValue(analyzer, out var analyzerMap) && analyzerMap.TryGetValue(key, out entry); @@ -28,8 +26,6 @@ public static bool TryGetValue(DiagnosticAnalyzer analyzer, (ProjectOrDocumentId public static void Cache(DiagnosticAnalyzer analyzer, (ProjectOrDocumentId key, string stateKey) key, CacheEntry entry) { - AssertKey(key); - // add new cache entry var analyzerMap = s_map.GetOrAdd(analyzer, _ => new ConcurrentDictionary<(ProjectOrDocumentId key, string stateKey), CacheEntry>(concurrencyLevel: 2, capacity: 10)); analyzerMap[key] = entry; @@ -37,7 +33,6 @@ public static void Cache(DiagnosticAnalyzer analyzer, (ProjectOrDocumentId key, public static void Remove(DiagnosticAnalyzer analyzer, (ProjectOrDocumentId key, string stateKey) key) { - AssertKey(key); // remove the entry if (!s_map.TryGetValue(analyzer, out var analyzerMap)) { @@ -57,10 +52,6 @@ public static void DropCache(DiagnosticAnalyzer analyzer) // drop any cache related to given analyzer s_map.TryRemove(analyzer, out _); } - - // make sure key is either documentId or projectId - private static void AssertKey((object key, string stateKey) key) - => Contract.ThrowIfFalse(key.key is DocumentId or ProjectId); } // in memory cache entry diff --git a/src/LanguageServer/Protocol/Handler/Diagnostics/ProjectOrDocumentId.cs b/src/LanguageServer/Protocol/Handler/Diagnostics/ProjectOrDocumentId.cs index ac1f4f63653a6..e9924b7e75bc3 100644 --- a/src/LanguageServer/Protocol/Handler/Diagnostics/ProjectOrDocumentId.cs +++ b/src/LanguageServer/Protocol/Handler/Diagnostics/ProjectOrDocumentId.cs @@ -10,18 +10,16 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; /// Wrapper around project and document ids for convenience in caching diagnostic results and /// use in the /// -internal readonly struct ProjectOrDocumentId +internal readonly record struct ProjectOrDocumentId { /// /// Non-null if this represents a documentId. Used for equality comparisons. /// - [SuppressMessage("CodeQuality", "IDE0052:Remove unread private members", Justification = "Used for equality comparison.")] private readonly DocumentId? _documentId; /// /// Non-null if this represents a projectId. Used for equality comparisons. /// - [SuppressMessage("CodeQuality", "IDE0052:Remove unread private members", Justification = "Used for equality comparison.")] private readonly ProjectId? _projectId; public ProjectOrDocumentId(ProjectId projectId)