Revert "UtilityAnalyzer: Use RegisterCompilationStartAction" #7262
Conversation
| @@ -31,9 +31,6 @@ public sealed class SonarCompilationStartAnalysisContext : SonarAnalysisContextB | |||
| public void RegisterSymbolAction(Action<SonarSymbolReportingContext> action, params SymbolKind[] symbolKinds) => | |||
| Context.RegisterSymbolAction(x => action(new(AnalysisContext, x)), symbolKinds); | |||
|
|
|||
| public void RegisterSemanticModelAction(Action<SonarSematicModelReportingContext> action) => | |||
| Context.RegisterSemanticModelAction(x => action(new(AnalysisContext, x))); | |||
There was a problem hiding this comment.
The point in time given by RegisterSemanticModelAction is still right. The binding phase needs to be finished, before you can successfully query the semantic model for symbols.
First the parse phase, where source is tokenized and parsed into syntax that follows the language grammar. Second the declaration phase, where declarations from source and imported metadata are analyzed to form named symbols. Next the bind phase, where identifiers in the code are matched to symbols.
We need these compiler pipeline entry points. Please do not remove any (we should instead add more). The right thing to do is to ignore the sematic model passed in RegisterSemanticModelAction and create a new one with Compilation.GetSemanticModel(syntaxTree) in the action instead.
There was a problem hiding this comment.
We need to revert the changes fully, as Only using a different semantic model will not fix the regression.
Calling ShouldGenerateMetrics from the SemanticModelAction is also slowing down the analysis significantly because it uses two concurrent collections inside.
The decision is to revert as close as possible to the original state in order to finish the sprint and make it to the next SQ release.
Making the change to SemanticModelAction makes sense, and at the same time it should be done as part of a proper sprint that focuses on it, so we can come up with the best-performing solution that is backed by a proper and thorough validation.
There was a problem hiding this comment.
@martin-strecker-sonarsource I agree with @csaba-sagi-sonarsource - one thing I learned from this sprint is that even if the change makes absolute sense in code and in theory and maybe for some project sample used to test locally we still need to do a thorough performance validation as it might behave in an unexpected way (like in this case).
From my point of view - I would add an issue for this and I'd tackle it in the next hardening (as this one is coming to an end tomorrow and there will be not enough time to validate).
I'll also add a card to do a sprint dedicated to the hardening of the UtilityAnalyzers (specified and planned as this one had only scope to fix the regression). I think we can bring performance and cleanup value this way.
mary-georgiou-sonarsource
changed the title
github-actions
bot
assigned costin-zaharia-sonarsource and unassigned mary-georgiou-sonarsource
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
LGTM!
The initial idea was to reuse the semantic models created during analysis (RegisterSemanticModelAction) instead of recreating them, on the compilation end by calling Compilation.GetSemanticModel() method.
In theory, this sounds very good and it's the recommended way by the compiler team. It practice it did not work as expected.
What we observed is that it added too much pressure on the task pool (too much thread contention) and it had the reverse effect and slowed everything down.
github-actions
bot
assigned mary-georgiou-sonarsource and unassigned costin-zaharia-sonarsource
|
Kudos, SonarCloud Quality Gate passed! |
github-actions
bot
moved this from Review in progress
to Review approved
in Best Kanban
This PR is reverting the change in the
UtilityAnalyzerbasefrom this PR as it caused a regression (Note: this is not a full revert).These are binlogs from the analysis of RavenDB without and with the revert.
