Investigate CSharpAvoidUninstantiatedInternalClasses performance

[Youssef1313]

In a binlog I was looking at, noticed:

It might be caused by SemanticModel usage below?

roslyn-analyzers/src/NetAnalyzers/CSharp/Microsoft.CodeQuality.Analyzers/Maintainability/CSharpAvoidUninstantiatedInternalClasses.cs

Lines 18 to 31 in 44af587

	context.RegisterSyntaxNodeAction(context =>
	{
	var usingDirective = (UsingDirectiveSyntax)context.Node;
	if (usingDirective.Alias != null &&
	usingDirective.DescendantNodes().OfType<GenericNameSyntax>().Any() &&
	context.SemanticModel.GetDeclaredSymbol(usingDirective) is IAliasSymbol aliasSymbol &&
	aliasSymbol.Target is INamedTypeSymbol namedTypeSymbol &&
	namedTypeSymbol.IsGenericType)
	{
	var generics = namedTypeSymbol.TypeParameters.Zip(namedTypeSymbol.TypeArguments, (parameter, argument) => (parameter, argument));
	ProcessGenericTypes(generics, instantiatedTypes);
	}
	}, SyntaxKind.UsingDirective);

Can this be improved?

cc @sharwell @mavasani

[mavasani]

Note that this rule is disabled by default though:

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidUninstantiatedInternalClasses.cs

Line 30 in 2928b85

RuleLevel.Disabled, // Code coverage tools provide superior results when done correctly.

[Youssef1313]

Note that this rule is disabled by default though:

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidUninstantiatedInternalClasses.cs

Line 30 in 2928b85

RuleLevel.Disabled, // Code coverage tools provide superior results when done correctly.

For the repo I got the binlog from, I have aggressively enabled all performance analyzers.

The code snippet I posted above is probably not the cause of performance issues. The semantic model is used only if there is a using directive of the form using X = C<..>. While this pattern exists in the repo, there are not too many occurrences.

#6309 is a possible improvement.

Another thing worth looking at is:

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidUninstantiatedInternalClasses.cs

Lines 116 to 119 in e22457d

	if (type.BaseType != null)
	{
	instantiatedTypes.TryAdd(type.BaseType, null);
	}

Here we try to add type.BaseType in the instantiatedTypes ConcurrentDictionary. I'm guessing we could have a lock contention here (or any other expensive work in TryAdd)?

I know that TryAdd internally tries to avoid contention by using a different lock for each bucket, but if we are passing the same type.BaseType, the same lock could be used.

For example:

public class C { }
public class C1 : C { }
public class C2 : C { }
public class C3 : C { }

If we analyze C1, C2, C3 concurrently, we'll call TryAdd for C and have a possible lock contention.

While I can't think of a way to avoid this for the above case, it can be avoided if the base type doesn't belong to compilation.Assembly. Adding such types to instantiatedTypes is unnecessary. We'll never report an error for such types and it's not important to know whether they are instantiated or not.

The very common scenario is when the base type is System.Object.

NOTE: All this is just guessing. I haven't profiled.

[Youssef1313]

                startContext.RegisterOperationAction(context =>
                {
                    var expr = (IObjectCreationOperation)context.Operation;
                    if (expr.Type is INamedTypeSymbol namedType)
                    {
                        instantiatedTypes.TryAdd(namedType, null);
                    }
                }, OperationKind.ObjectCreation);

This also might be a problem, if there are too many object creations of types not in current assembly, TryAdd might slow us down for the same reason.