-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make AnalyzerActions a value type #40691
Conversation
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticStartAnalysisScope.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticStartAnalysisScope.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticStartAnalysisScope.cs
Show resolved
Hide resolved
Done review pass (commit 7) |
The initially observed regression turned out to be a bug in .NET Core: dotnet/runtime#1713. |
Done review pass (commit 9) |
09d6be3
to
08981b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 13). @dotnet/roslyn-compiler (perhaps @agocke?) for a second review.
private ImmutableArray<OperationAnalyzerAction> _operationActions = ImmutableArray<OperationAnalyzerAction>.Empty; | ||
public static readonly AnalyzerActions Empty = new AnalyzerActions(concurrent: false); | ||
|
||
private ImmutableArray<CompilationStartAnalyzerAction> _compilationStartActions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just make these get-only autoprops?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Add*
methods mutate these fields in the current revision. A future change could make this struct immutable.
@@ -628,126 +628,187 @@ protected AnalyzerActions GetOrCreateAnalyzerActions(DiagnosticAnalyzer analyzer | |||
// moves from an analyzer-centric model to an action-centric model. For example, the driver would need to stop asking | |||
// if a particular analyzer can analyze syntax trees, and instead ask if any syntax tree actions are present. Also, | |||
// the driver needs to apply all relevant actions rather then applying the actions of individual analyzers. | |||
internal sealed class AnalyzerActions | |||
internal struct AnalyzerActions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a readonly struct with IsEmpty
and Concurrent moved to a With-ing pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is still a mutable struct. It would be a decent size change to convert all the remaining mutation methods to immutable transforms.
Fixes #39102
Reduces allocations during analysis of Roslyn.sln by many GB (6 GB, ~2.4%).
Baseline (c0c68fc):
Updated (c0c68fc + 4c37d9a)