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

Reduce allocations observed in local trace opening Roslyn.sln #68076

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion azure-pipelines-integration-corehost.yml
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ parameters:
- name: queueName
displayName: Queue Name
type: string
default: windows.vs2022preview.amd64.open
default: windows.vs2022preview.scout.amd64.open
values:
- windows.vs2022.amd64.open
- windows.vs2022.scout.amd64.open
Expand Down
14 changes: 11 additions & 3 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Text;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;
Expand Down Expand Up @@ -606,11 +607,18 @@ public static SpecialType GetSpecialTypeSafe(this TypeSymbol? type)
return type is object ? type.SpecialType : SpecialType.None;
}

public static bool IsAtLeastAsVisibleAs(this TypeSymbol type, Symbol sym, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
public static unsafe bool IsAtLeastAsVisibleAs(this TypeSymbol type, Symbol sym, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
CompoundUseSiteInfo<AssemblySymbol> localUseSiteInfo = useSiteInfo;
var result = type.VisitType((type1, symbol, unused) => IsTypeLessVisibleThan(type1, symbol, ref localUseSiteInfo), sym,
canDigThroughNullable: true); // System.Nullable is public
var localUseSiteInfoPtr = (nint)Unsafe.AsPointer(ref localUseSiteInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

var localUseSiteInfoPtr = (nint)Unsafe.AsPointer(ref localUseSiteInfo);

I am curious how much does this specific change save and how does it affect CPU time.

Copy link
Member Author

Choose a reason for hiding this comment

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

It saves 150MB allocations over 50 seconds (also mentioned in the commit message).

Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2023

Choose a reason for hiding this comment

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

It saves 150MB allocations over 50 seconds (also mentioned in the commit message).

Sorry, I am having trouble translating this into user observable terms.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed the IDE going slow opening Roslyn.sln, so I took a performance trace. Much of the CPU usage was in GC. When this was combined with high overall allocation rates, it suggested that the best course of action would be take a pass at reducing allocations that occurred during this trace. This was one of the allocations that was more easily addressed, in terms of it being a fairly localized change.

Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2023

Choose a reason for hiding this comment

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

This was one of the allocations that was more easily addressed, in terms of it being a fairly localized change.

Has this change, on its own, made any noticeable impact on the scenario? And I am not talking about number of allocations in the trace,

Copy link
Member Author

@sharwell sharwell May 4, 2023

Choose a reason for hiding this comment

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

I have no reason to believe so, even though the evidence suggests this was a small contributing factor to the much larger picture. I was only able to address 1% of the scenario allocations in the time box. However, over time we would hope that the incremental improvements would accumulate to the point of being observable.

Copy link
Contributor

Choose a reason for hiding this comment

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

var localUseSiteInfoPtr = (nint)Unsafe.AsPointer(ref localUseSiteInfo);

If we decide that we are comfortable going with this unsafe gymnastics, we should be able to get rid of the localUseSiteInfo local. It was added so that we could capture it in a closure.

Copy link
Member Author

Choose a reason for hiding this comment

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

The ref argument isn't guaranteed to be on the stack, so eliminating the local adds new gymnastics of a fixed statement (which ends up declaring a pointer to a non-blittable type and gives CS8500).

var result = type.VisitType(
static (type1, arg, _) =>
{
ref var localUseSiteInfo = ref Unsafe.AsRef<CompoundUseSiteInfo<AssemblySymbol>>((void*)arg.localUseSiteInfoPtr);
return IsTypeLessVisibleThan(type1, arg.sym, ref localUseSiteInfo);
},
(sym, localUseSiteInfoPtr),
canDigThroughNullable: true); // System.Nullable is public
Copy link
Member Author

Choose a reason for hiding this comment

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

⚠️ 🐉 Pay close attention to this change.

/cc @stephentoub

useSiteInfo = localUseSiteInfo;
return result is null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,7 @@ public static ImmutableDictionary<K, V> ToImmutableDictionaryOrEmpty<K, V>(this
return ImmutableDictionary.Create<K, V>(keyComparer);
}

return ImmutableDictionary.CreateRange(keyComparer, items);
return items.ToImmutableDictionary(keyComparer);
}

#nullable disable // Transpose doesn't handle empty arrays. Needs to be updated as appropriate.
Expand Down
13 changes: 7 additions & 6 deletions src/Compilers/Core/Portable/PEWriter/FullMetadataWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Reflection.Metadata.Ecma335;
using System.Threading;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.Emit;
using Roslyn.Utilities;
using EmitContext = Microsoft.CodeAnalysis.Emit.EmitContext;
Expand All @@ -28,7 +29,7 @@ internal sealed class FullMetadataWriter : MetadataWriter

private readonly Dictionary<ITypeDefinition, int> _fieldDefIndex;
private readonly Dictionary<ITypeDefinition, int> _methodDefIndex;
private readonly Dictionary<IMethodDefinition, int> _parameterListIndex;
private readonly SegmentedDictionary<IMethodDefinition, int> _parameterListIndex;

private readonly HeapOrReferenceIndex<AssemblyIdentity> _assemblyRefIndex;
private readonly HeapOrReferenceIndex<string> _moduleRefIndex;
Expand Down Expand Up @@ -101,7 +102,7 @@ private FullMetadataWriter(

_fieldDefIndex = new Dictionary<ITypeDefinition, int>(numTypeDefsGuess, ReferenceEqualityComparer.Instance);
_methodDefIndex = new Dictionary<ITypeDefinition, int>(numTypeDefsGuess, ReferenceEqualityComparer.Instance);
_parameterListIndex = new Dictionary<IMethodDefinition, int>(numMethods, ReferenceEqualityComparer.Instance);
_parameterListIndex = new SegmentedDictionary<IMethodDefinition, int>(numMethods, ReferenceEqualityComparer.Instance);

_assemblyRefIndex = new HeapOrReferenceIndex<AssemblyIdentity>(this);
_moduleRefIndex = new HeapOrReferenceIndex<string>(this);
Expand Down Expand Up @@ -430,13 +431,13 @@ private void CreateIndicesFor(IMethodDefinition methodDef)
private readonly struct DefinitionIndex<T> where T : class, IReference
{
// IReference to RowId
private readonly Dictionary<T, int> _index;
private readonly List<T> _rows;
private readonly SegmentedDictionary<T, int> _index;
private readonly SegmentedList<T> _rows;

public DefinitionIndex(int capacity)
{
_index = new Dictionary<T, int>(capacity, ReferenceEqualityComparer.Instance);
_rows = new List<T>(capacity);
_index = new SegmentedDictionary<T, int>(capacity, ReferenceEqualityComparer.Instance);
_rows = new SegmentedList<T>(capacity);
}

public bool TryGetValue(T item, out int rowId)
Expand Down
8 changes: 4 additions & 4 deletions src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ protected MetadataWriter(

// EDMAURER provide some reasonable size estimates for these that will avoid
// much of the reallocation that would occur when growing these from empty.
_signatureIndex = new Dictionary<ISignature, KeyValuePair<BlobHandle, ImmutableArray<byte>>>(module.HintNumberOfMethodDefinitions, ReferenceEqualityComparer.Instance); //ignores field signatures
_signatureIndex = new SegmentedDictionary<ISignature, KeyValuePair<BlobHandle, ImmutableArray<byte>>>(module.HintNumberOfMethodDefinitions, ReferenceEqualityComparer.Instance); //ignores field signatures

this.Context = context;
this.messageProvider = messageProvider;
Expand Down Expand Up @@ -431,14 +431,14 @@ private bool IsMinimalDelta

private readonly DynamicAnalysisDataWriter _dynamicAnalysisDataWriterOpt;

private readonly Dictionary<ICustomAttribute, BlobHandle> _customAttributeSignatureIndex = new Dictionary<ICustomAttribute, BlobHandle>();
private readonly SegmentedDictionary<ICustomAttribute, BlobHandle> _customAttributeSignatureIndex = new SegmentedDictionary<ICustomAttribute, BlobHandle>();
private readonly Dictionary<ITypeReference, BlobHandle> _typeSpecSignatureIndex = new Dictionary<ITypeReference, BlobHandle>(ReferenceEqualityComparer.Instance);
private readonly Dictionary<string, int> _fileRefIndex = new Dictionary<string, int>(32); // more than enough in most cases, value is a RowId
private readonly List<IFileReference> _fileRefList = new List<IFileReference>(32);
private readonly Dictionary<IFieldReference, BlobHandle> _fieldSignatureIndex = new Dictionary<IFieldReference, BlobHandle>(ReferenceEqualityComparer.Instance);
private readonly SegmentedDictionary<IFieldReference, BlobHandle> _fieldSignatureIndex = new SegmentedDictionary<IFieldReference, BlobHandle>(ReferenceEqualityComparer.Instance);

// We need to keep track of both the index of the signature and the actual blob to support VB static local naming scheme.
private readonly Dictionary<ISignature, KeyValuePair<BlobHandle, ImmutableArray<byte>>> _signatureIndex;
private readonly SegmentedDictionary<ISignature, KeyValuePair<BlobHandle, ImmutableArray<byte>>> _signatureIndex;

private readonly Dictionary<IMarshallingInformation, BlobHandle> _marshallingDescriptorIndex = new Dictionary<IMarshallingInformation, BlobHandle>();
protected readonly List<MethodImplementation> methodImplList = new List<MethodImplementation>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
cancellationToken As CancellationToken) As Diagnostic

' Diagnostic ids must be processed in case-insensitive fashion in VB.
Dim caseInsensitiveSpecificDiagnosticOptions =
ImmutableDictionary.Create(Of String, ReportDiagnostic)(CaseInsensitiveComparison.Comparer).AddRange(specificDiagnosticOptions)
Dim caseInsensitiveSpecificDiagnosticOptions = specificDiagnosticOptions.ToImmutableDictionary(CaseInsensitiveComparison.Comparer)

' Filter void diagnostics so that our callers don't have to perform resolution
' (which might copy the list of diagnostics).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ internal abstract class AbstractFileWriter

private readonly IDictionary<string, Node> _nodeMap;
private readonly IDictionary<string, TreeType> _typeMap;
private readonly Dictionary<string, string> _camelCaseMap = new(StringComparer.Ordinal);
private readonly List<string> _indentMap = new();

private const int INDENT_SIZE = 4;
private int _indentLevel;
Expand Down Expand Up @@ -94,7 +96,12 @@ private void WriteIndentIfNeeded()
{
if (_needIndent)
{
_writer.Write(new string(' ', _indentLevel * INDENT_SIZE));
while (_indentMap.Count <= _indentLevel)
{
_indentMap.Add(new string(' ', _indentMap.Count * INDENT_SIZE));
}

_writer.Write(_indentMap[_indentLevel]);
_needIndent = false;
}
}
Expand Down Expand Up @@ -260,13 +267,23 @@ protected static bool HasErrors(Node n)
return n.Errors == null || string.Compare(n.Errors, "true", true) == 0;
}

protected static string CamelCase(string name)
protected string CamelCase(string name)
{
if (_camelCaseMap.TryGetValue(name, out var camelCase))
return camelCase;

if (char.IsUpper(name[0]))
{
name = char.ToLowerInvariant(name[0]) + name.Substring(1);
camelCase = char.ToLowerInvariant(name[0]) + name.Substring(1);
}
return FixKeyword(name);
else
{
camelCase = name;
}

camelCase = FixKeyword(camelCase);
_camelCaseMap.Add(name, camelCase);
return camelCase;
}

protected static string FixKeyword(string name)
Expand Down
Loading