Remove potential leak in CachingSemanticModelProvider._providerCache #77192
Conversation
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 (dotnet/runtime#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.
ghost
added
Area-Analyzers
untriaged
333fred
left a comment
333fred
left a comment
There was a problem hiding this comment.
Do we have a bug to follow up on the general semantics of this type and either make it entirely static, or otherwise fix the issues with it?
|
I didn't log anything. As there is only one type that derives from SemanticModelProvider (CachingSemanticModelProvider), I considered condensing down to just a single class and making it static, but that seemed orthogonal to the memory leak. I don't think the current design is necessarily bad, it's just a bit more complex than it needs to be. Another concern is that ClearCache(Compilation) doesn't seem to be getting called, which sounds like an intentional decision. If we wish to maintain that, perhaps ClearCache(Compilation) should be removed so it doesn't claim to do cleanup that doesn't happen. In reply to: 2613565789 |
Let's please log something to follow up. |
|
|
@dotnet/roslyn-compiler for 2nd review |
| = new ConditionalWeakTable<Compilation, PerCompilationProvider>.CreateValueCallback(compilation => new PerCompilationProvider(compilation)); | ||
|
|
||
| private readonly ConditionalWeakTable<Compilation, PerCompilationProvider> _providerCache; | ||
| private static readonly ConditionalWeakTable<Compilation, PerCompilationProvider> s_providerCache = new ConditionalWeakTable<Compilation, PerCompilationProvider>(); |
There was a problem hiding this comment.
Why was this made static? Couldn't it remain an instance field with the same effect since the owner is a singleton now?
There was a problem hiding this comment.
The general guidance per @DustinCampbell is that CWT declarations should be static if possible to remove the cyclic leak possibility. But yes, it doesn't need to be static now that the owner is a singleton, but I figured it was best to do so.
| /// </summary> | ||
| internal sealed class CachingSemanticModelProvider : SemanticModelProvider | ||
| { | ||
| public static CachingSemanticModelProvider Instance { get; } = new CachingSemanticModelProvider(); |
There was a problem hiding this comment.
Consider adding a comment linking to the CWT issue so someone doesn't unintentionally undo this change in the future.
5 participants
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 (dotnet/runtime#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.