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

Reduce CFG memory allocations #5402

Merged
merged 2 commits into from
Feb 18, 2022
Merged

Reduce CFG memory allocations #5402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
931c734
Reduce CFG memory allocations
pavel-mikula-sonarsource Feb 16, 2022
1358666
Review update
pavel-mikula-sonarsource Feb 18, 2022
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
52 changes: 52 additions & 0 deletions analyzers/src/SonarAnalyzer.CFG/Extensions/PropertyInfoExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* 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;
using System.Collections;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;

namespace SonarAnalyzer.CFG
{
internal static class PropertyInfoExtensions
{
public static T ReadCached<T>(this PropertyInfo property, object instance, ref T cache) where T : class =>
cache ??= (T)property.GetValue(instance);
Copy link
Member

Choose a reason for hiding this comment

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

This will have the downside that, if a value is null, we will continue to ask for it, even if we requested it before. As long as this method proves better in practice, this is not a problem but we should be aware of it.

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, I considered that downside. For current usage, the BasicBlock.BranchValue is the hottest candidate to be null. And I assume it's going to be called 2 times per instance (once through OperationsAndBranchValue that cases the result, once for branching in the future).
The rest should be called zero or once (OriginalOperation, ExceptionType, Parent), or not be null (Children, Language, SemanticModel).


public static T ReadCached<T>(this PropertyInfo property, object instance, ref T? cache) where T : struct =>
cache ??= (T)property.GetValue(instance);

public static T ReadCached<T>(this PropertyInfo property, object instance, Func<object, T> createInstance, ref T cache) where T : class =>
cache ??= createInstance(property.GetValue(instance));

public static ImmutableArray<T> ReadCached<T>(this PropertyInfo property, object instance, ref ImmutableArray<T> cache) =>
ReadCached(property, instance, x => (T)x, ref cache);

public static ImmutableArray<T> ReadCached<T>(this PropertyInfo property, object instance, Func<object, T> createInstance, ref ImmutableArray<T> cache)
{
if (cache.IsDefault)
{
cache = ((IEnumerable)property.GetValue(instance)).Cast<object>().Select(createInstance).ToImmutableArray();
}
return cache;
}
}
}
16 changes: 0 additions & 16 deletions analyzers/src/SonarAnalyzer.CFG/Helpers/RoslynHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@
*/

using System;
using System.Collections;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;
using Microsoft.CodeAnalysis;

namespace SonarAnalyzer.CFG.Helpers
Expand All @@ -36,17 +32,5 @@ public static bool IsRoslynCfgSupported(int minimalVersion = MinimalSupportedMaj

public static Type FlowAnalysisType(string typeName) =>
typeof(SemanticModel).Assembly.GetType("Microsoft.CodeAnalysis.FlowAnalysis." + typeName);

public static Lazy<T> ReadValue<T>(this PropertyInfo property, object instance) =>
ReadValue(property, instance, x => (T)x);

public static Lazy<T> ReadValue<T>(this PropertyInfo property, object instance, Func<object, T> createInstance) =>
new Lazy<T>(() => createInstance(property.GetValue(instance)));

public static Lazy<ImmutableArray<T>> ReadImmutableArray<T>(this PropertyInfo property, object instance) =>
ReadImmutableArray(property, instance, x => (T)x);

public static Lazy<ImmutableArray<T>> ReadImmutableArray<T>(this PropertyInfo property, object instance, Func<object, T> createInstance) =>
new Lazy<ImmutableArray<T>>(() => ((IEnumerable)property.GetValue(instance)).Cast<object>().Select(createInstance).ToImmutableArray());
}
}
70 changes: 26 additions & 44 deletions analyzers/src/SonarAnalyzer.CFG/Roslyn/BasicBlock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ namespace SonarAnalyzer.CFG.Roslyn
{
public class BasicBlock
{
private static readonly ConditionalWeakTable<object, BasicBlock> InstanceCache = new ConditionalWeakTable<object, BasicBlock>();
private static readonly ConditionalWeakTable<object, BasicBlock> InstanceCache = new();
private static readonly PropertyInfo BranchValueProperty;
private static readonly PropertyInfo ConditionalSuccessorProperty;
private static readonly PropertyInfo ConditionKindProperty;
Expand All @@ -43,30 +43,31 @@ public class BasicBlock
private static readonly PropertyInfo OrdinalProperty;
private static readonly PropertyInfo PredecessorsProperty;

private readonly Lazy<IOperation> branchValue;
private readonly Lazy<ControlFlowBranch> conditionalSuccessor;
private readonly Lazy<ControlFlowConditionKind> conditionKind;
private readonly Lazy<ControlFlowRegion> enclosingRegion;
private readonly Lazy<ControlFlowBranch> fallThroughSuccessor;
private readonly Lazy<bool> isReachable;
private readonly Lazy<BasicBlockKind> kind;
private readonly Lazy<ImmutableArray<IOperation>> operations;
private readonly Lazy<int> ordinal;
private readonly Lazy<ImmutableArray<ControlFlowBranch>> predecessors;
private readonly object instance;
private readonly Lazy<ImmutableArray<ControlFlowBranch>> successors;
private readonly Lazy<ImmutableArray<BasicBlock>> successorBlocks;
private readonly Lazy<ImmutableArray<IOperation>> operationsAndBranchValue;
private IOperation branchValue;
private ControlFlowBranch conditionalSuccessor;
private ControlFlowConditionKind? conditionKind;
private ControlFlowRegion enclosingRegion;
private ControlFlowBranch fallThroughSuccessor;
private bool? isReachable;
private BasicBlockKind? kind;
private ImmutableArray<IOperation> operations;
private int? ordinal;
private ImmutableArray<ControlFlowBranch> predecessors;

public IOperation BranchValue => branchValue.Value;
public ControlFlowBranch ConditionalSuccessor => conditionalSuccessor.Value;
public ControlFlowConditionKind ConditionKind => conditionKind.Value;
public ControlFlowRegion EnclosingRegion => enclosingRegion.Value;
public ControlFlowBranch FallThroughSuccessor => fallThroughSuccessor.Value;
public bool IsReachable => isReachable.Value;
public BasicBlockKind Kind => kind.Value;
public ImmutableArray<IOperation> Operations => operations.Value;
public int Ordinal => ordinal.Value;
public ImmutableArray<ControlFlowBranch> Predecessors => predecessors.Value;
public IOperation BranchValue => BranchValueProperty.ReadCached(instance, ref branchValue);
public ControlFlowBranch ConditionalSuccessor => ConditionalSuccessorProperty.ReadCached(instance, ControlFlowBranch.Wrap, ref conditionalSuccessor);
public ControlFlowConditionKind ConditionKind => ConditionKindProperty.ReadCached(instance, ref conditionKind);
public ControlFlowRegion EnclosingRegion => EnclosingRegionProperty.ReadCached(instance, ControlFlowRegion.Wrap, ref enclosingRegion);
public ControlFlowBranch FallThroughSuccessor => FallThroughSuccessorProperty.ReadCached(instance, ControlFlowBranch.Wrap, ref fallThroughSuccessor);
public bool IsReachable => IsReachableProperty.ReadCached(instance, ref isReachable);
public BasicBlockKind Kind => KindProperty.ReadCached(instance, ref kind);
public ImmutableArray<IOperation> Operations => OperationsProperty.ReadCached(instance, ref operations);
public int Ordinal => OrdinalProperty.ReadCached(instance, ref ordinal);
public ImmutableArray<ControlFlowBranch> Predecessors => PredecessorsProperty.ReadCached(instance, ControlFlowBranch.Wrap, ref predecessors);
public ImmutableArray<ControlFlowBranch> Successors => successors.Value;
public ImmutableArray<BasicBlock> SuccessorBlocks => successorBlocks.Value;
public ImmutableArray<IOperation> OperationsAndBranchValue => operationsAndBranchValue.Value;
Expand All @@ -90,41 +91,22 @@ static BasicBlock()

private BasicBlock(object instance)
{
_ = instance ?? throw new ArgumentNullException(nameof(instance));
branchValue = BranchValueProperty.ReadValue<IOperation>(instance);
conditionalSuccessor = ConditionalSuccessorProperty.ReadValue(instance, ControlFlowBranch.Wrap);
conditionKind = ConditionKindProperty.ReadValue<ControlFlowConditionKind>(instance);
enclosingRegion = EnclosingRegionProperty.ReadValue(instance, ControlFlowRegion.Wrap);
fallThroughSuccessor = FallThroughSuccessorProperty.ReadValue(instance, ControlFlowBranch.Wrap);
isReachable = IsReachableProperty.ReadValue<bool>(instance);
kind = KindProperty.ReadValue<BasicBlockKind>(instance);
operations = OperationsProperty.ReadImmutableArray<IOperation>(instance);
ordinal = OrdinalProperty.ReadValue<int>(instance);
predecessors = PredecessorsProperty.ReadImmutableArray(instance, ControlFlowBranch.Wrap);
this.instance = instance ?? throw new ArgumentNullException(nameof(instance));
successors = new Lazy<ImmutableArray<ControlFlowBranch>>(() =>
{
// since Roslyn does not differentiate between pattern types in CFG, it builds unreachable block for missing
// pattern match even when discard pattern option is presented. In this case we explicitly exclude this branch
if (SwitchExpressionArmSyntaxWrapper.IsInstance(BranchValue?.Syntax) && DiscardPatternSyntaxWrapper.IsInstance(((SwitchExpressionArmSyntaxWrapper)BranchValue.Syntax).Pattern))
{
return FallThroughSuccessor != null ? ImmutableArray.Create(FallThroughSuccessor) : ImmutableArray<ControlFlowBranch>.Empty;
return FallThroughSuccessor is null ? ImmutableArray<ControlFlowBranch>.Empty : ImmutableArray.Create(FallThroughSuccessor);
}
else
{
return ImmutableArray.CreateRange(new[] { FallThroughSuccessor, ConditionalSuccessor }.Where(x => x != null));
return new[] { FallThroughSuccessor, ConditionalSuccessor }.Where(x => x != null).ToImmutableArray();
}
});
successorBlocks = new Lazy<ImmutableArray<BasicBlock>>(() => Successors.Select(x => x.Destination).Where(x => x != null).ToImmutableArray());
operationsAndBranchValue = new Lazy<ImmutableArray<IOperation>>(() =>
{
if (BranchValue == null)
{
return Operations;
}
var builder = Operations.ToBuilder();
builder.Add(BranchValue);
return builder.ToImmutable();
});
operationsAndBranchValue = new Lazy<ImmutableArray<IOperation>>(() => BranchValue is null ? Operations : Operations.Add(BranchValue));
}

public bool ContainsThrow() =>
Expand Down
44 changes: 18 additions & 26 deletions analyzers/src/SonarAnalyzer.CFG/Roslyn/ControlFlowBranch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ namespace SonarAnalyzer.CFG.Roslyn
{
public class ControlFlowBranch
{
private static readonly ConditionalWeakTable<object, ControlFlowBranch> InstanceCache = new ConditionalWeakTable<object, ControlFlowBranch>();
private static readonly ConditionalWeakTable<object, ControlFlowBranch> InstanceCache = new();
private static readonly PropertyInfo SourceProperty;
private static readonly PropertyInfo DestinationProperty;
private static readonly PropertyInfo SemanticsProperty;
Expand All @@ -37,21 +37,22 @@ public class ControlFlowBranch
private static readonly PropertyInfo LeavingRegionsProperty;
private static readonly PropertyInfo FinallyRegionsProperty;

private readonly Lazy<BasicBlock> source;
private readonly Lazy<BasicBlock> destination;
private readonly Lazy<ControlFlowBranchSemantics> semantics;
private readonly Lazy<bool> isConditionalSuccessor;
private readonly Lazy<ImmutableArray<ControlFlowRegion>> enteringRegions;
private readonly Lazy<ImmutableArray<ControlFlowRegion>> leavingRegions;
private readonly Lazy<ImmutableArray<ControlFlowRegion>> finallyRegions;
private readonly object instance;
private BasicBlock source;
private BasicBlock destination;
private ControlFlowBranchSemantics? semantics;
private bool? isConditionalSuccessor;
private ImmutableArray<ControlFlowRegion> enteringRegions;
private ImmutableArray<ControlFlowRegion> leavingRegions;
private ImmutableArray<ControlFlowRegion> finallyRegions;

public BasicBlock Source => source.Value;
public BasicBlock Destination => destination.Value;
public ControlFlowBranchSemantics Semantics => semantics.Value;
public bool IsConditionalSuccessor => isConditionalSuccessor.Value;
public ImmutableArray<ControlFlowRegion> EnteringRegions => enteringRegions.Value;
public ImmutableArray<ControlFlowRegion> LeavingRegions => leavingRegions.Value;
public ImmutableArray<ControlFlowRegion> FinallyRegions => finallyRegions.Value;
public BasicBlock Source => SourceProperty.ReadCached(instance, BasicBlock.Wrap, ref source);
public BasicBlock Destination => DestinationProperty.ReadCached(instance, BasicBlock.Wrap, ref destination);
public ControlFlowBranchSemantics Semantics => SemanticsProperty.ReadCached(instance, ref semantics);
public bool IsConditionalSuccessor => IsConditionalSuccessorProperty.ReadCached(instance, ref isConditionalSuccessor);
public ImmutableArray<ControlFlowRegion> EnteringRegions => EnteringRegionsProperty.ReadCached(instance, ControlFlowRegion.Wrap, ref enteringRegions);
public ImmutableArray<ControlFlowRegion> LeavingRegions => LeavingRegionsProperty.ReadCached(instance, ControlFlowRegion.Wrap, ref leavingRegions);
public ImmutableArray<ControlFlowRegion> FinallyRegions => FinallyRegionsProperty.ReadCached(instance, ControlFlowRegion.Wrap, ref finallyRegions);

static ControlFlowBranch()
{
Expand All @@ -67,17 +68,8 @@ static ControlFlowBranch()
}
}

private ControlFlowBranch(object instance)
{
_ = instance ?? throw new ArgumentNullException(nameof(instance));
source = SourceProperty.ReadValue(instance, BasicBlock.Wrap);
destination = DestinationProperty.ReadValue(instance, BasicBlock.Wrap);
semantics = SemanticsProperty.ReadValue<ControlFlowBranchSemantics>(instance);
isConditionalSuccessor = IsConditionalSuccessorProperty.ReadValue<bool>(instance);
enteringRegions = EnteringRegionsProperty.ReadImmutableArray(instance, ControlFlowRegion.Wrap);
leavingRegions = LeavingRegionsProperty.ReadImmutableArray(instance, ControlFlowRegion.Wrap);
finallyRegions = FinallyRegionsProperty.ReadImmutableArray(instance, ControlFlowRegion.Wrap);
}
private ControlFlowBranch(object instance) =>
this.instance = instance ?? throw new ArgumentNullException(nameof(instance));

public static ControlFlowBranch Wrap(object instance) =>
instance == null ? null : InstanceCache.GetValue(instance, x => new ControlFlowBranch(x));
Expand Down
27 changes: 11 additions & 16 deletions analyzers/src/SonarAnalyzer.CFG/Roslyn/ControlFlowGraph.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace SonarAnalyzer.CFG.Roslyn
{
public class ControlFlowGraph
{
private static readonly ConditionalWeakTable<object, ControlFlowGraph> InstanceCache = new ConditionalWeakTable<object, ControlFlowGraph>();
private static readonly ConditionalWeakTable<object, ControlFlowGraph> InstanceCache = new();
private static readonly PropertyInfo BlocksProperty;
private static readonly PropertyInfo LocalFunctionsProperty;
private static readonly PropertyInfo OriginalOperationProperty;
Expand All @@ -43,18 +43,18 @@ public class ControlFlowGraph
private static readonly MethodInfo GetLocalFunctionControlFlowGraphMethod;

private readonly object instance;
private readonly Lazy<ImmutableArray<BasicBlock>> blocks;
private readonly Lazy<ImmutableArray<IMethodSymbol>> localFunctions;
private readonly Lazy<ControlFlowGraph> parent;
private readonly Lazy<IOperation> originalOperation;
private readonly Lazy<ControlFlowRegion> root;
private ImmutableArray<BasicBlock> blocks;
private ImmutableArray<IMethodSymbol> localFunctions;
private ControlFlowGraph parent;
private IOperation originalOperation;
private ControlFlowRegion root;

public static bool IsAvailable { get; }
public ImmutableArray<BasicBlock> Blocks => blocks.Value;
public ImmutableArray<IMethodSymbol> LocalFunctions => localFunctions.Value;
public IOperation OriginalOperation => originalOperation.Value;
public ControlFlowGraph Parent => parent.Value;
public ControlFlowRegion Root => root.Value;
public ImmutableArray<BasicBlock> Blocks => BlocksProperty.ReadCached(instance, BasicBlock.Wrap, ref blocks);
public ImmutableArray<IMethodSymbol> LocalFunctions => LocalFunctionsProperty.ReadCached<IMethodSymbol>(instance, ref localFunctions);
public IOperation OriginalOperation => OriginalOperationProperty.ReadCached(instance, ref originalOperation);
public ControlFlowGraph Parent => ParentProperty.ReadCached(instance, Wrap, ref parent);
public ControlFlowRegion Root => RootProperty.ReadCached(instance, ControlFlowRegion.Wrap, ref root);
public BasicBlock EntryBlock => Blocks[Root.FirstBlockOrdinal];
public BasicBlock ExitBlock => Blocks[Root.LastBlockOrdinal];

Expand All @@ -78,11 +78,6 @@ static ControlFlowGraph()
private ControlFlowGraph(object instance)
{
this.instance = instance ?? throw new ArgumentNullException(nameof(instance));
blocks = BlocksProperty.ReadImmutableArray(instance, BasicBlock.Wrap);
localFunctions = LocalFunctionsProperty.ReadImmutableArray<IMethodSymbol>(instance);
originalOperation = OriginalOperationProperty.ReadValue<IOperation>(instance);
parent = ParentProperty.ReadValue(instance, Wrap);
root = RootProperty.ReadValue(instance, ControlFlowRegion.Wrap);
Debug.Assert(EntryBlock.Kind == BasicBlockKind.Entry, "Roslyn CFG Entry block is not the first one");
Debug.Assert(ExitBlock.Kind == BasicBlockKind.Exit, "Roslyn CFG Exit block is not the last one");
}
Expand Down
Loading