Skip to content
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

Performance: UtilityAnalyzerBase calls GetSemanticModel for each SyntaxTree #6558

Closed
martin-strecker-sonarsource opened this issue Dec 17, 2022 · 6 comments · Fixed by #6576 or #8412
Closed
Assignees
Labels
Area: C# C# rules related issues. Area: VB.NET VB.NET rules related issues. Type: Performance It takes too long.
Milestone

Comments

@martin-strecker-sonarsource
Copy link
Contributor

UtilityAnalyzerBase calls Compilation.GetSematicModel(syntaxTree) here

var treeMessages = c.Compilation.SyntaxTrees
.Where(x => ShouldGenerateMetrics(c, x))
.Select(x => CreateMessage(x, c.Compilation.GetSemanticModel(x)));

This is such a big performance issue that the Roslyn team wrote an analyzer RS1030 to flag its use. We should consider looking for a better approach as described by the message of that rule:

dotnet/roslyn-analyzers#3114 (comment)

@andrei-epure-sonarsource
Copy link
Contributor

andrei-epure-sonarsource commented Feb 8, 2023

(off-topic from this issue) @martin-strecker-sonarsource from the POV of RS1030, should we use RegisterSemanticModelAction for all our rules?

@martin-strecker-sonarsource
Copy link
Contributor Author

should we use RegisterSemanticModelAction for all our rules?

Not in general, but we should look for violations of RS1030.

There are some use cases where you have no other option than to call Compilation.GetSemanticModel(SyntaxTree). e.g. if you have an invocation node in the current tree and you get the IMethodSymbol of the invoked method and you call IMethodSymbol.DeclaringSyntaxReferences and you want to query some semantic model based on the syntax found, it might be that the method is defined in another file and therefore another SyntaxTree and you have to call Compilation.GetSemanticModel(SyntaxTree) to get the right SemanticModel. This is what GetSemanticModelOrDefault is for in our codebase.

public static SemanticModel GetSemanticModelOrDefault(this SyntaxTree syntaxTree, SemanticModel model)
{
// See https://github.com/dotnet/roslyn/issues/18730
if (syntaxTree == model.SyntaxTree)
{
return model;
}
return model.Compilation.ContainsSyntaxTree(syntaxTree)
? model.Compilation.GetSemanticModel(syntaxTree)
: null;
}

@martin-strecker-sonarsource
Copy link
Contributor Author

should we use RegisterSemanticModelAction for all our rules?

We should definitely think about some RegisterSyntaxTree analyzer. If we call the semanticModel in 99% of the cases for such a rule, we are better off with using RegisterSemanticModel instead. The reason is that "SyntxTreeAction" gets called right after the AST is produced. Then the compiler does in parallel the binding (which takes a long time). If you call the semanticModel, you will have to wait until the binding is finished basically blocking the thread.
If we use RegisterSemanticModel in such a case, no blocking occurs because the action is called when the binding is done.

I'm not really sure if the above is exactly right as the documentation doesn't tell about these specifics. The best I could find was this: https://github.com/dotnet/roslyn/blob/main/docs/analyzers/Analyzer%20Actions%20Semantics.md

@pavel-mikula-sonarsource
Copy link
Contributor

On Akka.Net, I did not see any change in performance, the semantic models are most likely cached as they are queried by different rules of the same analysis:
Before: 67s. 65s. 67s
After: 62s, 66s, 69s

@pavel-mikula-sonarsource
Copy link
Contributor

Results for lucenenet - small improvement
Before
1:48
1:29
1:30
1:35
1:34

After
1:47
1:27
1:31
1:28
1:29

@martin-strecker-sonarsource
Copy link
Contributor Author

Reopened

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Area: VB.NET VB.NET rules related issues. Type: Performance It takes too long.
Projects
None yet
5 participants