Reduce allocations during analysis result creation.#82139
Conversation
The local ImmutableHashSet usage in this method accounts for 0.9% of total allocations in the devhub process in the razor completion speedometer test. Local testing of mudblazor showed on average about 130 analyzers were added to this set per call. A standard HashSet is more efficient wrt memory usage and query performance than an ImmutableHashSet. The GetImmutableArray methods were modified to take in a IReadOnlyCollection instead of an ImmutableHashSet to maintain their contract of not being able to change that input set.
|
/pr-val |
|
View PR Validation Run triggered by @ToddGrun Parameters
|
I'm fine not wrapping in that case. Just doc they won't change the inputs. It's all internal. |
|
/pr-val |
|
View PR Validation Run triggered by @ToddGrun Parameters
|
| /// <param name="localDiagnosticsOpt">Diagnostic map to operate on</param> | ||
| private static ImmutableDictionary<TKey, ImmutableDictionary<DiagnosticAnalyzer, ImmutableArray<Diagnostic>>> GetImmutable<TKey>( | ||
| ImmutableHashSet<DiagnosticAnalyzer> analyzers, | ||
| HashSet<DiagnosticAnalyzer> analyzers, // Will not be modified |
There was a problem hiding this comment.
I changed this away from IReadOnlyCollection in commit 2 due to @CyrusNajmabadi 's request to just use HashSet and to comment it. I can change the PR comment to better reflect commit 2's state wrt this, or I can change back to what was done in commit 1.
There was a problem hiding this comment.
I changed this away from IReadOnlyCollection in commit 2
I am not suggesting to use this type
There was a problem hiding this comment.
HashSet doesn't derive from IReadOnlySet, but rather from IReadOnlyCollection.
There was a problem hiding this comment.
HashSet doesn't derive from IReadOnlySet, but rather from IReadOnlyCollection.
It does according to this page https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.hashset-1?view=net-10.0. Perhaps this depends on the target framework?
It looks like this doesn't reflect the final state of the PR |
|
Done with review pass (commit 2) |
|
@dotnet/roslyn-compiler For a second review |
|
/pr-val |
|
View PR Validation Run triggered by @ToddGrun Parameters
|
|
@dotnet/roslyn-compiler -- ptal for second review |
|
@dotnet/roslyn-compiler -- trivial change still in need of second review |
The local ImmutableHashSet usage in this method accounts for 0.9% of total allocations in the devhub process in the razor completion speedometer test. Local testing of mudblazor showed on average about 130 analyzers were added to this set per call. A standard HashSet is more efficient wrt memory usage and query performance than an ImmutableHashSet.
*** Previous allocations ***
