From 5ba8564b365a3446abc7fd939f4e4b952524e0c9 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Wed, 12 Feb 2025 14:12:49 -0800 Subject: [PATCH 1/2] Remove potential leak in CachingSemanticModelProvider._providerCache Anders recently shared a dmp where he was complaining about excessive memory usage in the Roslyn OOP process. Even after turning off FSA, he was still experiencing relatively high memory usage. Inspecting the dmp led to some questionable data in the CachingSemanticModelProvider._providerCache CWT. Luckily, Dustin was on the thread and mentioned a known issue in the CLR around CWTs (https://github.com/dotnet/runtime/issues/12255). This is indeed the case we are hitting, as the CWT's value could container a cycle with a pointer to itself (CachingSemanticModelProvider._providerCache -> PerCompilationProvider -> Compilation -> CachingSemanticModelProvider). The standard fix for this is to switch the CWT to static, and thus can't experience this cycle. This PR simply changes that CWT to static (and ensures that only a single CachingSemanticModelProvider is in use as it only uses static data to calculate it's result). Note that I kept in the ClearCache mechanism in this PR, even though it seems quite broken. Failure to call the ClearCache(compilation) method would have led to this leak, and indeed, this is what I experience when debugging locally. --- .../Portable/DiagnosticAnalyzer/AnalyzerDriver.cs | 2 +- .../CachingSemanticModelProvider.cs | 13 +++++++------ .../DiagnosticAnalyzer/CompilationWithAnalyzers.cs | 6 +++--- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs b/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs index 0e8e91913e027..0528bc9c60098 100644 --- a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs +++ b/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs @@ -844,7 +844,7 @@ internal static AnalyzerDriver CreateAndAttachToCompilation( { AnalyzerDriver analyzerDriver = compilation.CreateAnalyzerDriver(analyzers, analyzerManager, severityFilter); newCompilation = compilation - .WithSemanticModelProvider(new CachingSemanticModelProvider()) + .WithSemanticModelProvider(CachingSemanticModelProvider.Instance) .WithEventQueue(new AsyncQueue()); var categorizeDiagnostics = false; diff --git a/src/Compilers/Core/Portable/DiagnosticAnalyzer/CachingSemanticModelProvider.cs b/src/Compilers/Core/Portable/DiagnosticAnalyzer/CachingSemanticModelProvider.cs index 573045bbf5733..fbf39ecb53f61 100644 --- a/src/Compilers/Core/Portable/DiagnosticAnalyzer/CachingSemanticModelProvider.cs +++ b/src/Compilers/Core/Portable/DiagnosticAnalyzer/CachingSemanticModelProvider.cs @@ -23,22 +23,23 @@ namespace Microsoft.CodeAnalysis.Diagnostics /// internal sealed class CachingSemanticModelProvider : SemanticModelProvider { + public static CachingSemanticModelProvider Instance { get; } = new CachingSemanticModelProvider(); + private static readonly ConditionalWeakTable.CreateValueCallback s_createProviderCallback = new ConditionalWeakTable.CreateValueCallback(compilation => new PerCompilationProvider(compilation)); - private readonly ConditionalWeakTable _providerCache; + private static readonly ConditionalWeakTable s_providerCache = new ConditionalWeakTable(); - public CachingSemanticModelProvider() + private CachingSemanticModelProvider() { - _providerCache = new ConditionalWeakTable(); } public override SemanticModel GetSemanticModel(SyntaxTree tree, Compilation compilation, SemanticModelOptions options = default) - => _providerCache.GetValue(compilation, s_createProviderCallback).GetSemanticModel(tree, options); + => s_providerCache.GetValue(compilation, s_createProviderCallback).GetSemanticModel(tree, options); internal void ClearCache(SyntaxTree tree, Compilation compilation) { - if (_providerCache.TryGetValue(compilation, out var provider)) + if (s_providerCache.TryGetValue(compilation, out var provider)) { provider.ClearCachedSemanticModel(tree); } @@ -46,7 +47,7 @@ internal void ClearCache(SyntaxTree tree, Compilation compilation) internal void ClearCache(Compilation compilation) { - _providerCache.Remove(compilation); + s_providerCache.Remove(compilation); } private sealed class PerCompilationProvider diff --git a/src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs b/src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs index 45e7418d5887a..67c47debe0336 100644 --- a/src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs +++ b/src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs @@ -97,7 +97,7 @@ public CompilationWithAnalyzers(Compilation compilation, ImmutableArray()); _compilation = compilation; _analyzers = analyzers; @@ -723,7 +723,7 @@ private async Task ComputeAnalyzerDiagnosticsAsync(AnalysisScope? analysisScope, // subsequently discard this compilation. var compilation = analysisScope.IsSingleFileAnalysisForCompilerAnalyzer ? _compilation - : _compilation.WithSemanticModelProvider(new CachingSemanticModelProvider()).WithEventQueue(new AsyncQueue()); + : _compilation.WithSemanticModelProvider(CachingSemanticModelProvider.Instance).WithEventQueue(new AsyncQueue()); // Get the analyzer driver to execute analysis. using var driver = await CreateAndInitializeDriverAsync(compilation, _analysisOptions, analysisScope, _suppressors, categorizeDiagnostics: true, cancellationToken).ConfigureAwait(false); @@ -1188,7 +1188,7 @@ private static IEnumerable GetEffectiveDiagnosticsImpl(ImmutableArra if (compilation.SemanticModelProvider == null) { - compilation = compilation.WithSemanticModelProvider(new CachingSemanticModelProvider()); + compilation = compilation.WithSemanticModelProvider(CachingSemanticModelProvider.Instance); } var suppressMessageState = new SuppressMessageAttributeState(compilation); From 1f82a12a224a86d89be5eedc4ddb0a8cf6b45384 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Fri, 14 Feb 2025 05:20:11 -0800 Subject: [PATCH 2/2] Comment rationale behind making CWT static --- .../DiagnosticAnalyzer/CachingSemanticModelProvider.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Compilers/Core/Portable/DiagnosticAnalyzer/CachingSemanticModelProvider.cs b/src/Compilers/Core/Portable/DiagnosticAnalyzer/CachingSemanticModelProvider.cs index fbf39ecb53f61..4177c37847907 100644 --- a/src/Compilers/Core/Portable/DiagnosticAnalyzer/CachingSemanticModelProvider.cs +++ b/src/Compilers/Core/Portable/DiagnosticAnalyzer/CachingSemanticModelProvider.cs @@ -23,6 +23,9 @@ namespace Microsoft.CodeAnalysis.Diagnostics /// internal sealed class CachingSemanticModelProvider : SemanticModelProvider { + // Provide access to CachingSemanticModelProvider through a singleton. The inner CWT is static + // to avoid leak potential -- see https://github.com/dotnet/runtime/issues/12255. + // CachingSemanticModelProvider.s_providerCache -> PerCompilationProvider -> Compilation -> CachingSemanticModelProvider public static CachingSemanticModelProvider Instance { get; } = new CachingSemanticModelProvider(); private static readonly ConditionalWeakTable.CreateValueCallback s_createProviderCallback