Reduce string allocations from GetCategoryBasedDotnetAnalyzerDiagnosticSeverityKey
#72163
Conversation
dotnet-issue-labeler
bot
added
Area-Analyzers
untriaged
ghost
added
the
Community
Youssef1313
force-pushed
the
issues/72161
branch
from
5e50199 to
63001c5
Compare
|
@dotnet/dotnet-analyzers Please review |
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
…Extensions.cs Co-authored-by: Manish Vasani <[email protected]>
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
…Extensions.cs Co-authored-by: Jan Jones <[email protected]>
| private static string GetCategoryBasedDotnetAnalyzerDiagnosticSeverityKey(string category) | ||
| => $"{DotnetAnalyzerDiagnosticPrefix}.{CategoryPrefix}-{category}.{SeveritySuffix}"; | ||
| => s_categoryToSeverityKeyMap.GetOrAdd(category, category => $"{DotnetAnalyzerDiagnosticPrefix}.{CategoryPrefix}-{category}.{SeveritySuffix}"); |
There was a problem hiding this comment.
@AlekseyTs This method is all about constructing the key. The values for these keys are read from .editorconfig/.globalconfig from AnalyzerOptions. Not sure if that answers your question?
There was a problem hiding this comment.
So, the values are user controlled and a user will be able to grow s_categoryToSeverityKeyMap dictionary without any limit. Is this correct?
There was a problem hiding this comment.
@AlekseyTs The keys are defined by the analyzers themselves. I don't think it's realistic for a build to have too many analyzers with too many different categories. In fact, lots of analyzers share the same category.
There was a problem hiding this comment.
It looks like the dictionary is going to be alive for as long as the process is alive. It could be shared between different compilations. Theoretically, it might be possible to create more and more compilations to keep the dictionary grow. If that is the case, I would be more comfortable to have an explicit cap on the amount of keys cached (for example, keep certain amount of most recently used keys), or have the cache tied to a compilation (goes away once compilation goes away).
There was a problem hiding this comment.
Sharing between compilations was actually intended per review comment #72163 (comment)
Different compilations will still share most categories. I wouldn't expect all categories out there to exceed 20 or something.
There was a problem hiding this comment.
I wouldn't expect all categories out there to exceed 20 or something.
In a perfect world, yes. But we should also think about a worst case scenario. If we are certain about the amount of categories we will have to support, let's explicitly cap the size of the cache, let's say to 50 items. For example, we have ConcurrentLruCache type.
|
@Youssef1313 It looks like there are CI failures |
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
…Extensions.cs Co-authored-by: Jan Jones <[email protected]>
|
@Youssef1313 Thanks for the contribution |
Fixes #72161
The idea here is to avoid calling
GetCategoryBasedDotnetAnalyzerDiagnosticSeverityKeyin a loop. This is done by first callingGetCategoryBasedDotnetAnalyzerDiagnosticSeverityKeyonce before the loop, then passing it toTryGetSeverityFromBulkConfiguration