Skip to content

Commit

Permalink
Avoid using ConcurrentDictionary pools due to allocation overhead in …
Browse files Browse the repository at this point in the history
…Clear

Addresses 650MB allocations observed in https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1815692.
  • Loading branch information
sharwell committed May 19, 2023
1 parent 2420507 commit e995820
Show file tree
Hide file tree
Showing 7 changed files with 228 additions and 26 deletions.
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Collections.Immutable;
Expand Down Expand Up @@ -67,13 +67,13 @@ static void OnSymbolStart(
// has to be marked static, we want to report the diagnostic on the property/event.
// So we make a note of the property/event symbols which have at least one accessor with no instance access.
// At symbol end, we report candidate property/event symbols whose all accessors are candidates to be marked static.
var propertyOrEventCandidates = PooledConcurrentSet<ISymbol>.GetInstance();
var accessorCandidates = PooledConcurrentSet<IMethodSymbol>.GetInstance();
var propertyOrEventCandidates = TemporarySet<ISymbol>.Empty;
var accessorCandidates = TemporarySet<IMethodSymbol>.Empty;

var methodCandidates = PooledConcurrentSet<IMethodSymbol>.GetInstance();
var methodCandidates = TemporarySet<IMethodSymbol>.Empty;

// Do not flag methods that are used as delegates: https://github.com/dotnet/roslyn-analyzers/issues/1511
var methodsUsedAsDelegates = PooledConcurrentSet<IMethodSymbol>.GetInstance();
var methodsUsedAsDelegates = TemporarySet<IMethodSymbol>.Empty;

symbolStartContext.RegisterOperationAction(OnMethodReference, OperationKind.MethodReference);
symbolStartContext.RegisterOperationBlockStartAction(OnOperationBlockStart);
Expand All @@ -84,7 +84,7 @@ static void OnSymbolStart(
void OnMethodReference(OperationAnalysisContext operationContext)
{
var methodReference = (IMethodReferenceOperation)operationContext.Operation;
methodsUsedAsDelegates.Add(methodReference.Method);
methodsUsedAsDelegates.Add(methodReference.Method, operationContext.CancellationToken);
}

void OnOperationBlockStart(OperationBlockStartAnalysisContext blockStartContext)
Expand Down Expand Up @@ -125,8 +125,8 @@ void OnOperationBlockStart(OperationBlockStartAnalysisContext blockStartContext)
{
if (methodSymbol.IsAccessorMethod())
{
accessorCandidates.Add(methodSymbol);
propertyOrEventCandidates.Add(methodSymbol.AssociatedSymbol);
accessorCandidates.Add(methodSymbol, blockEndContext.CancellationToken);
propertyOrEventCandidates.Add(methodSymbol.AssociatedSymbol, blockEndContext.CancellationToken);
}
else if (methodSymbol.IsExternallyVisible())
{
Expand All @@ -137,17 +137,17 @@ void OnOperationBlockStart(OperationBlockStartAnalysisContext blockStartContext)
}
else
{
methodCandidates.Add(methodSymbol);
methodCandidates.Add(methodSymbol, blockEndContext.CancellationToken);
}
}
});
}

void OnSymbolEnd(SymbolAnalysisContext symbolEndContext)
{
foreach (var candidate in methodCandidates)
foreach (var candidate in methodCandidates.NonConcurrentEnumerable)
{
if (methodsUsedAsDelegates.Contains(candidate))
if (methodsUsedAsDelegates.Contains_NonConcurrent(candidate))
{
continue;
}
Expand All @@ -158,12 +158,12 @@ void OnSymbolEnd(SymbolAnalysisContext symbolEndContext)
}
}

foreach (var candidatePropertyOrEvent in propertyOrEventCandidates)
foreach (var candidatePropertyOrEvent in propertyOrEventCandidates.NonConcurrentEnumerable)
{
var allAccessorsAreCandidates = true;
foreach (var accessor in candidatePropertyOrEvent.GetAccessors())
{
if (!accessorCandidates.Contains(accessor) ||
if (!accessorCandidates.Contains_NonConcurrent(accessor) ||
IsOnObsoleteMemberChain(accessor, wellKnownTypeProvider))
{
allAccessorsAreCandidates = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,20 +54,20 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context)

void OnOperationBlockStart(OperationBlockStartAnalysisContext context)
{
var invocations = PooledConcurrentSet<IInvocationOperation>.GetInstance();
var invocations = TemporarySet<IInvocationOperation>.Empty;

context.RegisterOperationAction(context =>
{
var argument = (IArgumentOperation)context.Operation;
if (symbols.IsAnySubstringInvocation(argument.Value.WalkDownConversion(c => c.IsImplicit)) && argument.Parent is IInvocationOperation invocation)
{
invocations.Add(invocation);
invocations.Add(invocation, context.CancellationToken);
}
}, OperationKind.Argument);

context.RegisterOperationBlockEndAction(context =>
{
foreach (var invocation in invocations)
foreach (var invocation in invocations.NonConcurrentEnumerable)
{
// We search for an overload of the invoked member whose signature matches the signature of
// the invoked member, except with ReadOnlySpan<char> substituted in for some of the
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System.Collections.Immutable;
using Analyzer.Utilities;
Expand Down Expand Up @@ -106,8 +106,8 @@ void OnOperationBlockStart(OperationBlockStartAnalysisContext context)
// 2a. If the local reference is not a type int, bail out.
// 2b. If the local reference operation's parent is not a binary operation, add it to "localsToBailOut".
// 3. In an operation block end, we check if entries in "variableNameToOperationsMap" exist in "localToBailOut". If an entry is NOT present, we report a diagnostic at that invocation.
PooledConcurrentSet<ILocalSymbol> localsToBailOut = PooledConcurrentSet<ILocalSymbol>.GetInstance();
PooledConcurrentDictionary<ILocalSymbol, IInvocationOperation> variableNameToOperationsMap = PooledConcurrentDictionary<ILocalSymbol, IInvocationOperation>.GetInstance();
TemporarySet<ILocalSymbol> localsToBailOut = TemporarySet<ILocalSymbol>.Empty;
TemporaryDictionary<ILocalSymbol, IInvocationOperation> variableNameToOperationsMap = TemporaryDictionary<ILocalSymbol, IInvocationOperation>.Empty;

context.RegisterOperationAction(PopulateLocalReferencesSet, OperationKind.LocalReference);

Expand Down Expand Up @@ -137,7 +137,7 @@ void PopulateLocalReferencesSet(OperationAnalysisContext context)
}
}

localsToBailOut.Add(localReference.Local);
localsToBailOut.Add(localReference.Local, context.CancellationToken);
}

void AnalyzeInvocationOperation(OperationAnalysisContext context)
Expand All @@ -161,11 +161,11 @@ void AnalyzeInvocationOperation(OperationAnalysisContext context)
{
if (variableInitializer.Parent is IVariableDeclaratorOperation variableDeclaratorOperation)
{
variableNameToOperationsMap.TryAdd(variableDeclaratorOperation.Symbol, invocationOperation);
variableNameToOperationsMap.Add(variableDeclaratorOperation.Symbol, invocationOperation, context.CancellationToken);
}
else if (variableInitializer.Parent is IVariableDeclarationOperation variableDeclarationOperation && variableDeclarationOperation.Declarators.Length == 1)
{
variableNameToOperationsMap.TryAdd(variableDeclarationOperation.Declarators[0].Symbol, invocationOperation);
variableNameToOperationsMap.Add(variableDeclarationOperation.Declarators[0].Symbol, invocationOperation, context.CancellationToken);
}
}
}
Expand All @@ -188,7 +188,7 @@ static bool CheckOperatorKindAndOperand(IBinaryOperation binaryOperation, IOpera

void OnOperationBlockEnd(OperationBlockAnalysisContext context)
{
foreach (var variableNameAndLocation in variableNameToOperationsMap)
foreach (var variableNameAndLocation in variableNameToOperationsMap.NonConcurrentEnumerable)
{
ILocalSymbol variable = variableNameAndLocation.Key;
if (!localsToBailOut.Contains(variable))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ void OnOperationBlockStart(OperationBlockStartAnalysisContext context)
// already-reported violation.
// We also don't report any diagnostic if the concat operation has too many operands for the span-based
// Concat overloads to handle.
var topMostConcatOperations = PooledConcurrentSet<IBinaryOperation>.GetInstance();
var topMostConcatOperations = TemporarySet<IBinaryOperation>.Empty;

context.RegisterOperationAction(PopulateTopMostConcatOperations, OperationKind.Binary);
context.RegisterOperationBlockEndAction(ReportDiagnosticsOnRootConcatOperationsWithSubstringCalls);
Expand All @@ -82,7 +82,7 @@ void PopulateTopMostConcatOperations(OperationAnalysisContext context)
if (!TryGetTopMostConcatOperation(binary, out var topMostConcatOperation))
return;

topMostConcatOperations.Add(topMostConcatOperation);
topMostConcatOperations.Add(topMostConcatOperation, context.CancellationToken);
}

void ReportDiagnosticsOnRootConcatOperationsWithSubstringCalls(OperationBlockAnalysisContext context)
Expand All @@ -91,7 +91,7 @@ void ReportDiagnosticsOnRootConcatOperationsWithSubstringCalls(OperationBlockAna
// direct or conditional substring invocations when there is an applicable span-based overload of
// the string.Concat method.
// We don't report when the concatenation contains anything other than strings or character literals.
foreach (var operation in topMostConcatOperations)
foreach (var operation in topMostConcatOperations.NonConcurrentEnumerable)
{
if (ShouldBeReported(operation))
{
Expand Down
2 changes: 2 additions & 0 deletions src/Utilities/Compiler/Analyzer.Utilities.projitems
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@
<Compile Include="$(MSBuildThisFileDirectory)Options\AnalyzerOptionsExtensions.cs" />
<Compile Include="$(MSBuildThisFileDirectory)NullableAttributes.cs" />
<Compile Include="$(MSBuildThisFileDirectory)PooledObjects\PooledSortedSet.cs" />
<Compile Include="$(MSBuildThisFileDirectory)PooledObjects\TemporaryDictionary`2.cs" />
<Compile Include="$(MSBuildThisFileDirectory)PooledObjects\TemporarySet`1.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Range.cs" />
<Compile Include="$(MSBuildThisFileDirectory)RoslynHashCode.cs" />
<Compile Include="$(MSBuildThisFileDirectory)RoslynString.cs" />
Expand Down
89 changes: 89 additions & 0 deletions src/Utilities/Compiler/PooledObjects/TemporaryDictionary`2.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Threading;

namespace Analyzer.Utilities.PooledObjects
{
[SuppressMessage("Performance", "CA1815:Override equals and operator equals on value types", Justification = "Not used in this context")]
[SuppressMessage("Naming", "CA1711:Identifiers should not have incorrect suffix", Justification = "The 'Dictionary' suffix is intentional")]
internal struct TemporaryDictionary<TKey, TValue>
where TKey : notnull
{
#pragma warning disable CS0649 // Field 'TemporaryDictionary<TKey, TValue>.Empty' is never assigned to, and will always have its default value
public static readonly TemporaryDictionary<TKey, TValue> Empty;
#pragma warning restore CS0649 // Field 'TemporaryDictionary<TKey, TValue>.Empty' is never assigned to, and will always have its default value

/// <summary>
/// An empty dictionary used for creating non-null enumerators when no items have been added to the dictionary.
/// </summary>
private static readonly Dictionary<TKey, TValue> EmptyDictionary = new();

// 🐇 PERF: use PooledDictionary<TKey, TValue> instead of PooledConcurrentDictionary<TKey, TValue> due to
// allocation overhead in clearing the set for returning it to the pool.
private PooledDictionary<TKey, TValue>? _storage;

public readonly Enumerable NonConcurrentEnumerable
=> new(_storage ?? EmptyDictionary);

public void Free(CancellationToken cancellationToken)
{
Interlocked.Exchange(ref _storage, null)?.Free(cancellationToken);
}

private PooledDictionary<TKey, TValue> GetOrCreateStorage(CancellationToken cancellationToken)
{
if (_storage is not { } storage)
{
var newStorage = PooledDictionary<TKey, TValue>.GetInstance();
storage = Interlocked.CompareExchange(ref _storage, newStorage, null) ?? newStorage;
if (storage != newStorage)
{
// Another thread initialized the value. Make sure to release the unused object.
newStorage.Free(cancellationToken);
}
}

return storage;
}

internal void Add(TKey key, TValue value, CancellationToken cancellationToken)
{
var storage = GetOrCreateStorage(cancellationToken);
lock (storage)
{
storage.Add(key, value);
}
}

public readonly struct Enumerable
{
private readonly Dictionary<TKey, TValue> _dictionary;

public Enumerable(Dictionary<TKey, TValue> dictionary)
{
_dictionary = dictionary;
}

public Enumerator GetEnumerator()
=> new(_dictionary.GetEnumerator());
}

public struct Enumerator
{
private Dictionary<TKey, TValue>.Enumerator _enumerator;

public Enumerator(Dictionary<TKey, TValue>.Enumerator enumerator)
{
_enumerator = enumerator;
}

public bool MoveNext()
=> _enumerator.MoveNext();

public KeyValuePair<TKey, TValue> Current
=> _enumerator.Current;
}
}
}
111 changes: 111 additions & 0 deletions src/Utilities/Compiler/PooledObjects/TemporarySet`1.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Threading;

namespace Analyzer.Utilities.PooledObjects
{
[SuppressMessage("Performance", "CA1815:Override equals and operator equals on value types", Justification = "Not used in this context")]
internal struct TemporarySet<T>
{
#pragma warning disable CS0649 // Field 'TemporarySet<T>.Empty' is never assigned to, and will always have its default value
public static readonly TemporarySet<T> Empty;
#pragma warning restore CS0649 // Field 'TemporarySet<T>.Empty' is never assigned to, and will always have its default value

/// <summary>
/// An empty set used for creating non-null enumerators when no items have been added to the set.
/// </summary>
private static readonly HashSet<T> EmptyHashSet = new();

// 🐇 PERF: use PooledHashSet<T> instead of PooledConcurrentSet<T> due to allocation overhead in
// clearing the set for returning it to the pool.
private PooledHashSet<T>? _storage;

public readonly Enumerable NonConcurrentEnumerable
=> new(_storage ?? EmptyHashSet);

public void Free(CancellationToken cancellationToken)
{
Interlocked.Exchange(ref _storage, null)?.Free(cancellationToken);
}

private PooledHashSet<T> GetOrCreateStorage(CancellationToken cancellationToken)
{
if (_storage is not { } storage)
{
var newStorage = PooledHashSet<T>.GetInstance();
storage = Interlocked.CompareExchange(ref _storage, newStorage, null) ?? newStorage;
if (storage != newStorage)
{
// Another thread initialized the value. Make sure to release the unused object.
newStorage.Free(cancellationToken);
}
}

return storage;
}

public bool Add(T item, CancellationToken cancellationToken)
{
var storage = GetOrCreateStorage(cancellationToken);
lock (storage)
{
return storage.Add(item);
}
}

public readonly bool Contains(T item)
{
if (_storage is not { } storage)
return false;

lock (storage)
{
return storage.Contains(item);
}
}

public readonly bool Contains_NonConcurrent(T item)
{
if (_storage is not { } storage)
return false;

return storage.Contains(item);
}

public readonly Enumerator GetEnumerator_NonConcurrent()
{
return new Enumerator((_storage ?? EmptyHashSet).GetEnumerator());
}

public readonly struct Enumerable
{
private readonly HashSet<T> _set;

public Enumerable(HashSet<T> set)
{
_set = set;
}

public Enumerator GetEnumerator()
=> new(_set.GetEnumerator());
}

public struct Enumerator
{
private HashSet<T>.Enumerator _enumerator;

public Enumerator(HashSet<T>.Enumerator enumerator)
{
_enumerator = enumerator;
}

public bool MoveNext()
=> _enumerator.MoveNext();

public T Current
=> _enumerator.Current;
}
}
}

0 comments on commit e995820

Please sign in to comment.