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.

Sign up for GitHub

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

Cache CreateCfg #5403

Merged
merged 1 commit into from
Feb 18, 2022
Merged

Cache CreateCfg #5403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions analyzers/src/SonarAnalyzer.CFG/Roslyn/ControlFlowGraphCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2022 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Linq;
using System.Runtime.CompilerServices;
using Microsoft.CodeAnalysis;
using SonarAnalyzer.Extensions;
using StyleCop.Analyzers.Lightup;

namespace SonarAnalyzer.CFG.Roslyn
{
public abstract class ControlFlowGraphCacheBase
{
private readonly ConditionalWeakTable<SyntaxNode, Wrapper> cache = new();

protected abstract bool HasNestedCfg(SyntaxNode node);
protected abstract bool IsLocalFunction(SyntaxNode node);

public ControlFlowGraph FindOrCreate(SyntaxNode declaration, SemanticModel model)
{
var rootSyntax = model.GetOperation(declaration).RootOperation().Syntax;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand how this is meant to work. If we take the example from CreateCfg_Performance_UsesCache_CS and we request the CFG for the lambda, we will receive the CFG of the root operation this is the method declaration. Is this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. Roslyn doesn't let us directly create CFG for Lambda/Local function. We need to create a CFG from the root (method). And find our desired CFG in the CFG tree.

That's also the reason for the high cache hit rate.

var wrapper = cache.GetValue(rootSyntax, x => new Wrapper(ControlFlowGraph.Create(x, model)));
if (HasNestedCfg(declaration))
{
// We need to go up and track all possible enclosing lambdas, local functions and other FlowAnonymousFunctionOperations
foreach (var node in declaration.AncestorsAndSelf().TakeWhile(x => x != rootSyntax).Reverse())
{
if (IsLocalFunction(node))
{
wrapper = new(wrapper.Cfg.GetLocalFunctionControlFlowGraph(node));
}
else if (wrapper.FlowOperation(node) is { WrappedOperation: not null } flowOperation)
{
wrapper = new(wrapper.Cfg.GetAnonymousFunctionControlFlowGraph(flowOperation));
}
else if (node == declaration)
{
return null; // Lambda syntax is not always recognized as a FlowOperation for invalid syntaxes
}
}
}
return wrapper.Cfg;
}

private sealed class Wrapper
{
private IFlowAnonymousFunctionOperationWrapper[] flowOperations;

public ControlFlowGraph Cfg { get; }

public Wrapper(ControlFlowGraph cfg) =>
Cfg = cfg;

public IFlowAnonymousFunctionOperationWrapper FlowOperation(SyntaxNode node)
{
flowOperations ??= Cfg.FlowAnonymousFunctionOperations().ToArray(); // Avoid recomputing, it's expensive and called many times for a single CFG
return flowOperations.SingleOrDefault(x => x.WrappedOperation.Syntax == node);
}
}
}
}
44 changes: 14 additions & 30 deletions analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ namespace SonarAnalyzer.Extensions
{
internal static partial class SyntaxNodeExtensions
{
private static readonly ControlFlowGraphCache CfgCache = new();

public static ControlFlowGraph CreateCfg(this SyntaxNode body, SemanticModel model) =>
CfgCache.FindOrCreate(body.Parent, model);

public static bool ContainsConditionalConstructs(this SyntaxNode node) =>
node != null &&
node.DescendantNodes()
Expand Down Expand Up @@ -113,36 +118,6 @@ public static SyntaxNode RemoveParentheses(this SyntaxNode expression)
return current;
}

public static ControlFlowGraph CreateCfg(this SyntaxNode body, SemanticModel semanticModel)
{
var operation = semanticModel.GetOperation(body.Parent);
var rootSyntax = operation.RootOperation().Syntax;
var cfg = ControlFlowGraph.Create(rootSyntax, semanticModel);
if (body.Parent.IsAnyKind(SyntaxKindEx.LocalFunctionStatement, SyntaxKind.SimpleLambdaExpression, SyntaxKind.AnonymousMethodExpression, SyntaxKind.ParenthesizedLambdaExpression))
{
// We need to go up and track all possible enclosing lambdas, local functions and other FlowAnonymousFunctionOperations
var cfgFlowOperations = cfg.FlowAnonymousFunctionOperations().ToArray(); // Avoid recomputing for ancestors that do not produce FlowAnonymousFunction
foreach (var node in body.Parent.AncestorsAndSelf().TakeWhile(x => x != rootSyntax).Reverse())
{
if (node.IsKind(SyntaxKindEx.LocalFunctionStatement))
{
cfg = cfg.GetLocalFunctionControlFlowGraph(node);
cfgFlowOperations = cfg.FlowAnonymousFunctionOperations().ToArray();
}
else if (cfgFlowOperations.SingleOrDefault(x => x.WrappedOperation.Syntax == node) is { WrappedOperation: not null } flowOperation)
{
cfg = cfg.GetAnonymousFunctionControlFlowGraph(flowOperation);
cfgFlowOperations = cfg.FlowAnonymousFunctionOperations().ToArray();
}
else if (node == body)
{
return null;
}
}
}
return cfg;
}

private static string GetUnknownType(SyntaxKind kind)
{
#if DEBUG
Expand All @@ -151,5 +126,14 @@ private static string GetUnknownType(SyntaxKind kind)
return "type";
#endif
}

private sealed class ControlFlowGraphCache : ControlFlowGraphCacheBase
{
protected override bool IsLocalFunction(SyntaxNode node) =>
node.IsKind(SyntaxKindEx.LocalFunctionStatement);

protected override bool HasNestedCfg(SyntaxNode node) =>
node.IsAnyKind(SyntaxKindEx.LocalFunctionStatement, SyntaxKind.SimpleLambdaExpression, SyntaxKind.AnonymousMethodExpression, SyntaxKind.ParenthesizedLambdaExpression);
}
}
}
34 changes: 11 additions & 23 deletions analyzers/src/SonarAnalyzer.VisualBasic/Extensions/SyntaxNodeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.VisualBasic;
using Microsoft.CodeAnalysis.VisualBasic.Syntax;
Expand All @@ -29,6 +28,11 @@ namespace SonarAnalyzer.Extensions
{
internal static partial class SyntaxNodeExtensions
{
private static readonly ControlFlowGraphCache CfgCache = new();

public static ControlFlowGraph CreateCfg(this SyntaxNode block, SemanticModel model) =>
CfgCache.FindOrCreate(block, model);

public static bool IsPartOfBinaryNegationOrCondition(this SyntaxNode node)
{
if (!(node.Parent is MemberAccessExpressionSyntax))
Expand Down Expand Up @@ -56,29 +60,13 @@ public static object FindConstantValue(this SyntaxNode node, SemanticModel seman
public static string FindStringConstant(this SyntaxNode node, SemanticModel semanticModel) =>
FindConstantValue(node, semanticModel) as string;

public static ControlFlowGraph CreateCfg(this SyntaxNode block, SemanticModel semanticModel)
private sealed class ControlFlowGraphCache : ControlFlowGraphCacheBase
{
var operation = semanticModel.GetOperation(block);
var rootSyntax = operation.RootOperation().Syntax;
var cfg = ControlFlowGraph.Create(rootSyntax, semanticModel);
if (block is LambdaExpressionSyntax)
{
// We need to go up and track all possible enclosing lambdas and other FlowAnonymousFunctionOperations
var cfgFlowOperations = cfg.FlowAnonymousFunctionOperations().ToArray(); // Avoid recomputing for ancestors that do not produce FlowAnonymousFunction
foreach (var node in block.AncestorsAndSelf().TakeWhile(x => x != rootSyntax).Reverse())
{
if (cfgFlowOperations.SingleOrDefault(x => x.WrappedOperation.Syntax == node) is { WrappedOperation: not null } flowOperation)
{
cfg = cfg.GetAnonymousFunctionControlFlowGraph(flowOperation);
cfgFlowOperations = cfg.FlowAnonymousFunctionOperations().ToArray();
}
else if (node == block)
{
return null; // Lambda syntax is not always recognized as FlowOperation for invalid syntaxes
}
}
}
return cfg;
protected override bool IsLocalFunction(SyntaxNode node) =>
false;

protected override bool HasNestedCfg(SyntaxNode node) =>
node is LambdaExpressionSyntax;
}
}
}
65 changes: 65 additions & 0 deletions analyzers/tests/SonarAnalyzer.UnitTest/Extensions/SyntaxNodeExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@

extern alias csharp;
extern alias vbnet;

using System;
using System.Linq;
using FluentAssertions;
using FluentAssertions.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Operations;
Expand Down Expand Up @@ -330,6 +333,68 @@ public void CreateCfg_InvalidSyntax_ReturnsCfg_CS(string code)
SyntaxNodeExtensionsCS.CreateCfg(lambda.Body, model).Should().NotBeNull();
}

[TestMethod]
public void CreateCfg_Performance_UsesCache_CS()
{
const string code = @"
using System;
public class Sample
{
public void Main(string noiseToHaveMoreOperations)
{
noiseToHaveMoreOperations.ToString();
noiseToHaveMoreOperations.ToString();
noiseToHaveMoreOperations.ToString();
void LocalFunction()
{
noiseToHaveMoreOperations.ToString();
noiseToHaveMoreOperations.ToString();
noiseToHaveMoreOperations.ToString();
Action a = () => 42.ToString();
}
}
}";
var (tree, model) = TestHelper.CompileCS(code);
var lambda = tree.GetRoot().DescendantNodes().OfType<CS.ParenthesizedLambdaExpressionSyntax>().Single();
Action a = () =>
{
for (var i = 0; i < 10000; i++)
{
SyntaxNodeExtensionsCS.CreateCfg(lambda, model);
}
};
a.ExecutionTime().Should().BeLessThan(1.Seconds()); // Takes roughly 0.2 sec on CI
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried to run without the cache? I'm curious about the improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roughly 5x slower on my machine without cache on this very small method. The bigger the method is, the worse it gets.

I used the full user repro project build to see how it works. It went down from 8 minutes to 20 sec with the cache.

}

[TestMethod]
public void CreateCfg_Performance_UsesCache_VB()
{
const string code = @"
Public Class Sample
Public Sub Main(NoiseToHaveMoreOperations As String)
NoiseToHaveMoreOperations.ToString()
NoiseToHaveMoreOperations.ToString()
NoiseToHaveMoreOperations.ToString()
Dim Outer As Action = Sub()
NoiseToHaveMoreOperations.ToString()
NoiseToHaveMoreOperations.ToString()
NoiseToHaveMoreOperations.ToString()
Dim Inner As Action = Sub() NoiseToHaveMoreOperations.ToString()
End Sub
End Sub
End Class";
var (tree, model) = TestHelper.CompileVB(code);
var lambda = tree.GetRoot().DescendantNodes().OfType<VB.SingleLineLambdaExpressionSyntax>().Single();
Action a = () =>
{
for (var i = 0; i < 10000; i++)
{
SyntaxNodeExtensionsCS.CreateCfg(lambda, model);
}
};
a.ExecutionTime().Should().BeLessThan(1.Seconds()); // Takes roughly 0.4 sec on CI
}

private static SyntaxToken GetFirstTokenOfKind(SyntaxTree syntaxTree, SyntaxKind kind) =>
syntaxTree.GetRoot().DescendantTokens().First(token => token.IsKind(kind));
}
Expand Down