Async lightbulb performance improvement for compiler analyzer execution#56844
Async lightbulb performance improvement for compiler analyzer execution#56844mavasani merged 13 commits intodotnet:mainfrom
Conversation
Prior to this change, compiler diagnostic analyzer could only be run from the IDE for entire document span. This is primarily due to lack of an analyzer API that allows the analyzer to register a span-based semantic diagnostic callback. Though we can consider designing and adding such an analyzer API, it would be purely an IDE-only analyzer action, as analyzers are never executed for a span in batch compilation mode - this would make it difficult to justify adding such an API. With this PR, we add a workaround in the analyzer driver to allow executing compiler analyzer's semantic model action scoped to a filter span. This should speed up executing the compiler analyzer for lightbulb scenarios in the IDE, which are always scoped to current line span. Verified that both the compiler and IDE tests added with this PR fail prior to the product changes. **NOTE:** This change would not give us any perceivable improvement in non-async lightbulb scenario, as we are extremely likely to have at least one other analyzer that needs to run on the entire document. The performance improvement should be perceivable in async lightbulb scenario as the high priority code fixes bucket (add usings and merge conflict resolution) only run the compiler analyzer.
| var declDiagnostics = context.SemanticModel.GetDeclarationDiagnostics(context.FilterSpan, context.CancellationToken); | ||
| var bodyDiagnostics = context.SemanticModel.GetMethodBodyDiagnostics(context.FilterSpan, context.CancellationToken); |
There was a problem hiding this comment.
This is where we get the core performance boost.
| Dim diagnostic = Assert.Single(diagnostics) | ||
| Assert.Equal("CS0219", diagnostic.Id) | ||
| End Using | ||
| End Function |
There was a problem hiding this comment.
similar point about testing different spans.
There was a problem hiding this comment.
@CyrusNajmabadi Adding the IDE test for testing different spans failed as the IDE seems to be querying the compiler diagnostics for entire containing member instead of just the line span:
roslyn/src/Features/Core/Portable/Diagnostics/DocumentAnalysisExecutor.cs
Lines 251 to 284 in 4fc7245
This adjusted span workaround seems to be added for #1557, which is pretty old and has no failing tests or scenarios. I am going to just remove that adjusted span computation code and address any fallout subsequently based on failing scenarios.
There was a problem hiding this comment.
So attempting to remove GetAdjustedSpanForCompilerAnalyzerAsync workaround leads to few integration test failures. So the claim in #1557 is true - invoking SemanticModel.Get*Diagnostics(lineSpan) with just the current line span instead of entire method body span indeed does not return all the diagnostics for the current line. I have reverted the commit that removed the workaround and instead adjusted the above test to also account for this fact.
@333fred @jcouv - #1557 does represent a potential perf improvement case for light bulb - IDE is currently being forced to always fetch compiler semantic diagnostics for entire method body span instead of line span due to this bug. If compiler team were to fix this, IDE can remove this workaround and see a perf improvement by requesting the diagnostics just for the current line span.
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Overall looks good. I don't know the compiler side well enough to be confident there though.
… into AsyncLightbulbPerf
|
@dotnet/roslyn-compiler for reviews |
|
Done review pass (commit 11). |
|
@dotnet/roslyn-compiler for an additional review. |
| } | ||
|
|
||
| return builder.ToImmutable(); | ||
| if (needsSpanBasedCompilationUnitCompletedEvent) |
There was a problem hiding this comment.
We will only reach here if compilation has not completed for the entire compilation unit. The whole idea here is to generate a synthesized event for the compilation unit that can trigger semantic model actions with partial tree span before the real compilation unit completed event is generated. We can probably use a different type to represent such synthesized events instead of CompilationUnitCompletedEvent, but then we would have to duplicate the code in the analyzer driver that invokes semantic model actions for compilation unit completed event and this new event type.
…CompletedEvent.cs
Addresses the first bullet point in #56843.
Prior to this change, compiler diagnostic analyzer could only be run from the IDE for entire document span. This is primarily due to lack of an analyzer API that allows the analyzer to register a span-based semantic diagnostic callback. Though we can consider designing and adding such an analyzer API, it would be purely an IDE-only analyzer action, as analyzers are never executed for a span in batch compilation mode - this would make it difficult to justify adding such an API.
With this PR, we add a workaround in the analyzer driver to allow executing compiler analyzer's semantic model action scoped to a filter span. This should speed up executing the compiler analyzer for lightbulb scenarios in the IDE, which are always scoped to current line span.
Verified that both the compiler and IDE tests added with this PR fail prior to the product changes.
NOTE: This change would not give us any perceivable improvement in non-async lightbulb scenario, as we are extremely likely to have at least one other analyzer that needs to run on the entire document. The performance improvement should be perceivable in async lightbulb scenario as the high priority code fixes bucket (add usings and merge conflict resolution) only run the compiler analyzer.