Analyzer driver performance improvements#46508
Conversation
|
@AlekseyTs - this is the performance optimization in analyzer driver that we talked about a while back. We now hook up a semantic model provider to the compilation when executing analyzers, such that the analyzer driver controls the lifetime and caching of the shared semantic model with bound nodes. |
|
Also tagging @gafter - this change drops the weak reference to semantic model with cached bound nodes in |
…lationEvent to avoid unnecessary concurrent dictionary access on hot path
…g `SuppressMessageAttributeState.IsDiagnosticSuppressed` - fixes some unit tests
|
i don't know this subsystem well enough to review. but i strongly approve of us doing something here. teh design seems sane, and i think it's good that it becomes an explicit 'owned' things by a provider, rather than an impl detail that depends on weak-refs. |
It looks like we are missing an overload for this API. I assume for VB as well. #Closed Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs:2105 in f087408. [](commit_id = f087408, deletion_comment = False) |
Thanks @AlekseyTs. I have addressed the feedback and also added unit tests for the same. |
…com/mavasani/roslyn into AnalyzerDriverSemanticModelProvider
|
Will take a look now, thanks. I was waiting on the nullable changes. |
| _semanticModel = newSemanticModel; | ||
| Symbol = symbol; | ||
| SemanticModelWithCachedBoundNodes = semanticModelWithCachedBoundNodes; | ||
| _lazyCachedDeclaringReferences = new Lazy<ImmutableArray<SyntaxReference>>(() => symbol.DeclaringSyntaxReferences); |
There was a problem hiding this comment.
_lazyCachedDeclaringReferences [](start = 12, length = 30)
Consider making this a lazily-initialized ImmutableArray with ImmutableInterlocked.Initialize, instead of using a Lazy<T>.
There was a problem hiding this comment.
This is existing code, so I was trying to not change it in this PR.
There was a problem hiding this comment.
Consider either doing it inline, or filling an issue to follow up. We already do perf-sensitive work in this class, and this is a very-commonly-allocated class type.
In reply to: 469652219 [](ancestors = 469652219)
|
Done review pass (commit 15) #Closed |
|
Done review pass (commit 16). One minor comment about delegate caching, but this approach with the CWT looks very promising and I'll be happy to have it :) |
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 17). Thanks Manish!
|
@jaredpar Any further feedback? |
|
Thanks for the reviews! |
This change improves the analyzer execution performance by plugging in an optional semantic model provider into the compilation. This allows analyzer callbacks to fully share semantic model and bound nodes with the compiler binding phase. Additionally, analyzers invoking
compilation.GetSemanticModelalso get the shared semantic model, further improving performance. Analyzer driver ensures that we do not hold onto the cached semantic model/bound nodes for the entire lifetime of the compilation by dropping the cached semantic model after processing theCompilationUnitCompletedEventfor each syntax tree, which indicates the entire syntax tree's binding and analysis phase has completed.Performance measurements on the compiler benchmark suite for analyzer execution and real world repo shows ~10-15% improvement in analyzer execution time. There is also a large allocation improvement (~30%) in analyzer execution, though majority of this chunk comes from caching some delegates that were being created on hot paths, rather then from sharing semantic model through this provider.
Strongly recommended to review commit-by-commit
Commit 1 (f06d87c): Nullable enable analyzer driver. I also have a separate PR out for this commit, if that is easier to review: Nullable enable analyzer driver #46409
Commit 2 (039c424): Semantic model provider implementation for the performance improvements
Performance numbers
1. Compiler benchmark
Performance suite: https://github.com/dotnet/roslyn/blob/master/docs/wiki/Measuring-Compiler-Performance.md
Master branch
This PR branch
2. Building real world repo with analyzers
Scenario: Rebuild of RoslynAnalyzers.sln https://github.com/dotnet/roslyn-analyzers repo