From b3afe69bdf676b02362d9e9c32bb979b2890d151 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 31 Aug 2020 05:29:05 +0100 Subject: [PATCH 1/7] Devirtualize TokenMap --- .../Core/Portable/CodeGen/TokenMap.cs | 94 ++++++++----- .../Portable/Emit/CommonPEModuleBuilder.cs | 6 +- .../Emit/MetadataEntityReferenceComparer.cs | 50 ------- .../Core/Portable/IReferenceOrISignature.cs | 133 ++++++++++++++++++ .../Core/Portable/PEWriter/MetadataWriter.cs | 14 +- .../Portable/PEWriter/ReferenceIndexerBase.cs | 17 +-- 6 files changed, 203 insertions(+), 111 deletions(-) delete mode 100644 src/Compilers/Core/Portable/Emit/MetadataEntityReferenceComparer.cs create mode 100644 src/Compilers/Core/Portable/IReferenceOrISignature.cs diff --git a/src/Compilers/Core/Portable/CodeGen/TokenMap.cs b/src/Compilers/Core/Portable/CodeGen/TokenMap.cs index 49df657529796..713c43608c742 100644 --- a/src/Compilers/Core/Portable/CodeGen/TokenMap.cs +++ b/src/Compilers/Core/Portable/CodeGen/TokenMap.cs @@ -7,9 +7,8 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.Diagnostics; -using Microsoft.CodeAnalysis.PooledObjects; -using Roslyn.Utilities; +using Microsoft.Cci; +using System.Threading; namespace Microsoft.CodeAnalysis.CodeGen { @@ -24,74 +23,91 @@ namespace Microsoft.CodeAnalysis.CodeGen /// internal sealed class TokenMap { - private readonly ConcurrentDictionary _itemIdentityToToken = new ConcurrentDictionary(ReferenceEqualityComparer.Instance); + private readonly ConcurrentDictionary _itemIdentityToToken = new ConcurrentDictionary(); + private readonly Dictionary _itemEquivalentToToken = new Dictionary(); + private object[] _items = Array.Empty(); + private int _count = 0; - private readonly Dictionary _itemToToken; - private readonly ArrayBuilder _items = new ArrayBuilder(); + internal TokenMap() + { + } - internal TokenMap(IEqualityComparer comparer) + public uint GetOrAddTokenFor(IReference item, out bool referenceAdded) { - _itemToToken = new Dictionary(comparer); + if (_itemIdentityToToken.TryGetValue(new IReferenceOrISignature(item), out uint token)) + { + referenceAdded = false; + return token; + } + + return AddItem(new IReferenceOrISignatureEquivalent(item), out referenceAdded); } - public uint GetOrAddTokenFor(object item, out bool referenceAdded) + public uint GetOrAddTokenFor(ISignature item, out bool referenceAdded) { - uint tmp; - if (_itemIdentityToToken.TryGetValue(item, out tmp)) + if (_itemIdentityToToken.TryGetValue(new IReferenceOrISignature(item), out uint token)) { referenceAdded = false; - return (uint)tmp; + return token; } - return AddItem(item, out referenceAdded); + return AddItem(new IReferenceOrISignatureEquivalent(item), out referenceAdded); } - private uint AddItem(object item, out bool referenceAdded) + private uint AddItem(IReferenceOrISignatureEquivalent item, out bool referenceAdded) { - Debug.Assert(item is Cci.ISignature || item is Cci.IReference); uint token; - // NOTE: cannot use GetOrAdd here since items and itemToToken must be in sync // so if we do need to add we have to take a lock and modify both collections. - lock (_items) + lock (_itemEquivalentToToken) { - if (!_itemToToken.TryGetValue(item, out token)) + // Check if there is an equivalent type that has a token + if (!_itemEquivalentToToken.TryGetValue(item, out token)) { - token = (uint)_items.Count; - _items.Add(item); - _itemToToken.Add(item, token); + token = (uint)_count; + // No equivalent, add the token for this type + _itemEquivalentToToken.Add(item, token); + + var count = (int)token + 1; + var items = _items; + if (items.Length > count) + { + items[(int)token] = item.AsObject(); + } + else + { + // Not enough room, we need to resize the array + Array.Resize(ref items, Math.Max(8, count * 2)); + items[(int)token] = item.AsObject(); + + // Update the updated array reference before updating _count + Volatile.Write(ref _items, items); + } + + Volatile.Write(ref _count, count); } } + // Use the provided token to update the reference dictionary referenceAdded = _itemIdentityToToken.TryAdd(item, token); return token; } public object GetItem(uint token) { - lock (_items) - { - return _items[(int)token]; - } - } - - public IEnumerable GetAllItems() - { - lock (_items) - { - return _items.ToArray(); - } + return _items[(int)token]; } //TODO: why is this is called twice during emit? // should probably return ROA instead of IE and cache that in Module. (and no need to return count) - public IEnumerable GetAllItemsAndCount(out int count) + public object[] GetAllItems() { - lock (_items) - { - count = _items.Count; - return _items.ToArray(); - } + // Read the count prior to getting the array + int count = Volatile.Read(ref _count); + object[] items = Volatile.Read(ref _items); + + // Return a right sized copy of the array + return (new ReadOnlySpan(items, 0, count)).ToArray(); } } } diff --git a/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs b/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs index 6fff9647c5f2e..b7026b7a96695 100644 --- a/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs +++ b/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs @@ -33,7 +33,7 @@ internal abstract class CommonPEModuleBuilder : Cci.IUnit, Cci.IModuleReference internal Cci.IMethodReference DebugEntryPoint; private readonly ConcurrentDictionary _methodBodyMap; - private readonly TokenMap _referencesInILMap = new TokenMap(MetadataEntityReferenceComparer.ConsiderEverything); + private readonly TokenMap _referencesInILMap = new TokenMap(); private readonly ItemTokenMap _stringsInILMap = new ItemTokenMap(); private readonly ItemTokenMap _sourceDocumentsInILMap = new ItemTokenMap(); @@ -335,9 +335,9 @@ public string GetStringFromToken(uint token) return _stringsInILMap.GetItem(token); } - public IEnumerable ReferencesInIL(out int count) + public object[] ReferencesInIL() { - return _referencesInILMap.GetAllItemsAndCount(out count); + return _referencesInILMap.GetAllItems(); } /// diff --git a/src/Compilers/Core/Portable/Emit/MetadataEntityReferenceComparer.cs b/src/Compilers/Core/Portable/Emit/MetadataEntityReferenceComparer.cs deleted file mode 100644 index 90ccd7b0cc107..0000000000000 --- a/src/Compilers/Core/Portable/Emit/MetadataEntityReferenceComparer.cs +++ /dev/null @@ -1,50 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System.Collections.Generic; -using Microsoft.CodeAnalysis.Symbols; - -namespace Microsoft.CodeAnalysis.Emit -{ - internal sealed class MetadataEntityReferenceComparer : IEqualityComparer - { - internal static readonly MetadataEntityReferenceComparer ConsiderEverything = new MetadataEntityReferenceComparer(TypeCompareKind.ConsiderEverything); - - private readonly TypeCompareKind _compareKind; - - private MetadataEntityReferenceComparer(TypeCompareKind compareKind) - { - _compareKind = compareKind; - } - - public new bool Equals(object x, object y) - { - if (x is null) - { - return y is null; - } - else if (ReferenceEquals(x, y)) - { - return true; - } - else if (x is ISymbolInternal sx && y is ISymbolInternal sy) - { - return sx.Equals(sy, _compareKind); - } - else if (x is ISymbolCompareKindComparableInternal cx && y is ISymbolCompareKindComparableInternal cy) - { - return cx.Equals(cy, _compareKind); - } - else - { - return x.Equals(y); - } - } - - public int GetHashCode(object obj) - { - return obj?.GetHashCode() ?? 0; - } - } -} diff --git a/src/Compilers/Core/Portable/IReferenceOrISignature.cs b/src/Compilers/Core/Portable/IReferenceOrISignature.cs new file mode 100644 index 0000000000000..ab17e34270959 --- /dev/null +++ b/src/Compilers/Core/Portable/IReferenceOrISignature.cs @@ -0,0 +1,133 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#nullable enable + +using System; +using System.Collections.Generic; +using Microsoft.Cci; +using Microsoft.CodeAnalysis.Symbols; + +namespace Microsoft.CodeAnalysis +{ + /// + /// Used to devirtualize Dictionary/HashSet for EqualityComparer{T}.Default + /// + internal struct IReferenceOrISignatureEquivalent : IEquatable + { + private readonly object? _item; + + public IReferenceOrISignatureEquivalent(IReference item) + { + _item = item; + } + + public IReferenceOrISignatureEquivalent(ISignature item) + { + _item = item; + } + + public IReferenceOrISignatureEquivalent(IMethodReference item) + { + _item = item; + } + + public bool Equals(IReferenceOrISignatureEquivalent other) + { + object? x = _item; + object? y = other._item; + if (x is null) + { + return y is null; + } + else if (ReferenceEquals(x, y)) + { + return true; + } + + return Equals(x, y); + } + + private new static bool Equals(object? x, object? y) + { + if (x is null) + { + return y is null; + } + else if (ReferenceEquals(x, y)) + { + return true; + } + else if (x is ISymbolInternal sx && y is ISymbolInternal sy) + { + return sx.Equals(sy, TypeCompareKind.ConsiderEverything); + } + else if (x is ISymbolCompareKindComparableInternal cx && y is ISymbolCompareKindComparableInternal cy) + { + return cx.Equals(cy, TypeCompareKind.ConsiderEverything); + } + else + { + return x.Equals(y); + } + } + + public override bool Equals(object? obj) + { + return obj is IReferenceOrISignatureEquivalent refOrSig ? + Equals(refOrSig) : + Equals(_item, obj); + } + + public override int GetHashCode() => _item?.GetHashCode() ?? 0; + + public override string ToString() => _item?.ToString() ?? "null"; + + internal object AsObject() => _item!; + } + + /// + /// Used to devirtualize ConcurrentDictionary for EqualityComparer{T}.Default and ReferenceEquals + /// + internal struct IReferenceOrISignature : IEquatable + { + private readonly object? _item; + + public static implicit operator IReferenceOrISignature(IReferenceOrISignatureEquivalent d) => new IReferenceOrISignature(d.AsObject()); + + private IReferenceOrISignature(object? item) + { + _item = item; + } + + public IReferenceOrISignature(IReference item) + { + _item = item; + } + + public IReferenceOrISignature(ISignature item) + { + _item = item; + } + + public bool Equals(IReferenceOrISignature other) + { + object? x = _item; + object? y = other._item; + + return ReferenceEquals(x, y); + } + + public override bool Equals(object? obj) + { + return obj is IReferenceOrISignature refOrSig ? + Equals(refOrSig) : + ReferenceEquals(_item, obj); + } + + public override int GetHashCode() => _item?.GetHashCode() ?? 0; + + public override string ToString() => _item?.ToString() ?? "null"; + } +} diff --git a/src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs b/src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs index 42c0e6557ef2a..9615cceff8d00 100644 --- a/src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs +++ b/src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs @@ -464,18 +464,10 @@ private bool IsMinimalDelta private void CreateMethodBodyReferenceIndex() { - int count; - var referencesInIL = module.ReferencesInIL(out count); + var referencesInIL = module.ReferencesInIL(); - _pseudoSymbolTokenToTokenMap = new EntityHandle[count]; - _pseudoSymbolTokenToReferenceMap = new object[count]; - - int cur = 0; - foreach (object o in referencesInIL) - { - _pseudoSymbolTokenToReferenceMap[cur] = o; - cur++; - } + _pseudoSymbolTokenToTokenMap = new EntityHandle[referencesInIL.Length]; + _pseudoSymbolTokenToReferenceMap = referencesInIL; } private void CreateIndices() diff --git a/src/Compilers/Core/Portable/PEWriter/ReferenceIndexerBase.cs b/src/Compilers/Core/Portable/PEWriter/ReferenceIndexerBase.cs index 49feb2b198973..3a5a7a510a4d3 100644 --- a/src/Compilers/Core/Portable/PEWriter/ReferenceIndexerBase.cs +++ b/src/Compilers/Core/Portable/PEWriter/ReferenceIndexerBase.cs @@ -6,14 +6,15 @@ using System.Collections.Generic; using System.Diagnostics; using EmitContext = Microsoft.CodeAnalysis.Emit.EmitContext; +using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Emit; namespace Microsoft.Cci { internal abstract class ReferenceIndexerBase : MetadataVisitor { - private readonly HashSet _alreadySeen = new HashSet(MetadataEntityReferenceComparer.ConsiderEverything); - private readonly HashSet _alreadyHasToken = new HashSet(MetadataEntityReferenceComparer.ConsiderEverything); + private readonly HashSet _alreadySeen = new HashSet(); + private readonly HashSet _alreadyHasToken = new HashSet(); protected bool typeReferenceNeedsToken; internal ReferenceIndexerBase(EmitContext context) @@ -46,7 +47,7 @@ public override void Visit(IEventDefinition eventDefinition) public override void Visit(IFieldReference fieldReference) { - if (!_alreadySeen.Add(fieldReference)) + if (!_alreadySeen.Add(new IReferenceOrISignatureEquivalent(fieldReference))) { return; } @@ -125,7 +126,7 @@ public override void Visit(IMethodReference methodReference) return; } - if (!_alreadySeen.Add(methodReference)) + if (!_alreadySeen.Add(new IReferenceOrISignatureEquivalent(methodReference))) { return; } @@ -393,7 +394,7 @@ public override void Visit(ITypeReference typeReference) // Returns true if we need to look at the children, false otherwise. private bool VisitTypeReference(ITypeReference typeReference) { - if (!_alreadySeen.Add(typeReference)) + if (!_alreadySeen.Add(new IReferenceOrISignatureEquivalent(typeReference))) { if (!this.typeReferenceNeedsToken) { @@ -401,7 +402,7 @@ private bool VisitTypeReference(ITypeReference typeReference) } this.typeReferenceNeedsToken = false; - if (!_alreadyHasToken.Add(typeReference)) + if (!_alreadyHasToken.Add(new IReferenceOrISignatureEquivalent(typeReference))) { return false; } @@ -419,13 +420,13 @@ private bool VisitTypeReference(ITypeReference typeReference) if (specializedNestedTypeReference != null) { INestedTypeReference unspecializedNestedTypeReference = specializedNestedTypeReference.GetUnspecializedVersion(Context); - if (_alreadyHasToken.Add(unspecializedNestedTypeReference)) + if (_alreadyHasToken.Add(new IReferenceOrISignatureEquivalent(unspecializedNestedTypeReference))) { RecordTypeReference(unspecializedNestedTypeReference); } } - if (this.typeReferenceNeedsToken && _alreadyHasToken.Add(typeReference)) + if (this.typeReferenceNeedsToken && _alreadyHasToken.Add(new IReferenceOrISignatureEquivalent(typeReference))) { RecordTypeReference(typeReference); } From 7a9aee7711cfca5495775110abac305442bb15f6 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 31 Aug 2020 14:20:43 +0100 Subject: [PATCH 2/7] Apply suggestions from code review Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com> --- src/Compilers/Core/Portable/CodeGen/TokenMap.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Compilers/Core/Portable/CodeGen/TokenMap.cs b/src/Compilers/Core/Portable/CodeGen/TokenMap.cs index 713c43608c742..55df6fd2c6224 100644 --- a/src/Compilers/Core/Portable/CodeGen/TokenMap.cs +++ b/src/Compilers/Core/Portable/CodeGen/TokenMap.cs @@ -7,8 +7,8 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using Microsoft.Cci; using System.Threading; +using Microsoft.Cci; namespace Microsoft.CodeAnalysis.CodeGen { @@ -23,8 +23,8 @@ namespace Microsoft.CodeAnalysis.CodeGen /// internal sealed class TokenMap { - private readonly ConcurrentDictionary _itemIdentityToToken = new ConcurrentDictionary(); - private readonly Dictionary _itemEquivalentToToken = new Dictionary(); + private readonly ConcurrentDictionary _itemIdentityToToken = new(); + private readonly Dictionary _itemEquivalentToToken = new(); private object[] _items = Array.Empty(); private int _count = 0; From 86dcb4b4c04ff76a3445b818b7e16393af3f4397 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 31 Aug 2020 17:25:09 +0100 Subject: [PATCH 3/7] Target typed new() --- src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs | 6 +++--- .../Core/Portable/PEWriter/ReferenceIndexerBase.cs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs b/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs index b7026b7a96695..40db5f70a6f43 100644 --- a/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs +++ b/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs @@ -33,9 +33,9 @@ internal abstract class CommonPEModuleBuilder : Cci.IUnit, Cci.IModuleReference internal Cci.IMethodReference DebugEntryPoint; private readonly ConcurrentDictionary _methodBodyMap; - private readonly TokenMap _referencesInILMap = new TokenMap(); - private readonly ItemTokenMap _stringsInILMap = new ItemTokenMap(); - private readonly ItemTokenMap _sourceDocumentsInILMap = new ItemTokenMap(); + private readonly TokenMap _referencesInILMap = new(); + private readonly ItemTokenMap _stringsInILMap = new(); + private readonly ItemTokenMap _sourceDocumentsInILMap = new(); private ImmutableArray _lazyAssemblyReferenceAliases; private ImmutableArray _lazyManagedResources; diff --git a/src/Compilers/Core/Portable/PEWriter/ReferenceIndexerBase.cs b/src/Compilers/Core/Portable/PEWriter/ReferenceIndexerBase.cs index 3a5a7a510a4d3..7ed9dc12d73a4 100644 --- a/src/Compilers/Core/Portable/PEWriter/ReferenceIndexerBase.cs +++ b/src/Compilers/Core/Portable/PEWriter/ReferenceIndexerBase.cs @@ -13,8 +13,8 @@ namespace Microsoft.Cci { internal abstract class ReferenceIndexerBase : MetadataVisitor { - private readonly HashSet _alreadySeen = new HashSet(); - private readonly HashSet _alreadyHasToken = new HashSet(); + private readonly HashSet _alreadySeen = new(); + private readonly HashSet _alreadyHasToken = new(); protected bool typeReferenceNeedsToken; internal ReferenceIndexerBase(EmitContext context) From 726231373b26d439ee1932b21c5d781f2effd28e Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 31 Aug 2020 19:22:34 +0100 Subject: [PATCH 4/7] Apply suggestions from code review Co-authored-by: Jared Parsons --- src/Compilers/Core/Portable/IReferenceOrISignature.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compilers/Core/Portable/IReferenceOrISignature.cs b/src/Compilers/Core/Portable/IReferenceOrISignature.cs index ab17e34270959..0ede4d03869fb 100644 --- a/src/Compilers/Core/Portable/IReferenceOrISignature.cs +++ b/src/Compilers/Core/Portable/IReferenceOrISignature.cs @@ -14,7 +14,7 @@ namespace Microsoft.CodeAnalysis /// /// Used to devirtualize Dictionary/HashSet for EqualityComparer{T}.Default /// - internal struct IReferenceOrISignatureEquivalent : IEquatable + internal readonly struct IReferenceOrISignatureEquivalent : IEquatable { private readonly object? _item; @@ -90,7 +90,7 @@ public override bool Equals(object? obj) /// /// Used to devirtualize ConcurrentDictionary for EqualityComparer{T}.Default and ReferenceEquals /// - internal struct IReferenceOrISignature : IEquatable + internal readonly struct IReferenceOrISignature : IEquatable { private readonly object? _item; From 658fbe3a7db6a2d0d4f045e4e9815f00048f5860 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 31 Aug 2020 19:23:20 +0100 Subject: [PATCH 5/7] Return ReadOnlySpan --- src/Compilers/Core/Portable/CodeGen/TokenMap.cs | 4 ++-- src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs | 2 +- src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Compilers/Core/Portable/CodeGen/TokenMap.cs b/src/Compilers/Core/Portable/CodeGen/TokenMap.cs index 55df6fd2c6224..1f9f102941800 100644 --- a/src/Compilers/Core/Portable/CodeGen/TokenMap.cs +++ b/src/Compilers/Core/Portable/CodeGen/TokenMap.cs @@ -100,14 +100,14 @@ public object GetItem(uint token) //TODO: why is this is called twice during emit? // should probably return ROA instead of IE and cache that in Module. (and no need to return count) - public object[] GetAllItems() + public ReadOnlySpan GetAllItems() { // Read the count prior to getting the array int count = Volatile.Read(ref _count); object[] items = Volatile.Read(ref _items); // Return a right sized copy of the array - return (new ReadOnlySpan(items, 0, count)).ToArray(); + return new ReadOnlySpan(items, 0, count); } } } diff --git a/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs b/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs index 40db5f70a6f43..00bf03d15d0d0 100644 --- a/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs +++ b/src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs @@ -335,7 +335,7 @@ public string GetStringFromToken(uint token) return _stringsInILMap.GetItem(token); } - public object[] ReferencesInIL() + public ReadOnlySpan ReferencesInIL() { return _referencesInILMap.GetAllItems(); } diff --git a/src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs b/src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs index 9615cceff8d00..b61dc5a33baef 100644 --- a/src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs +++ b/src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs @@ -467,7 +467,7 @@ private void CreateMethodBodyReferenceIndex() var referencesInIL = module.ReferencesInIL(); _pseudoSymbolTokenToTokenMap = new EntityHandle[referencesInIL.Length]; - _pseudoSymbolTokenToReferenceMap = referencesInIL; + _pseudoSymbolTokenToReferenceMap = referencesInIL.ToArray(); } private void CreateIndices() From 7753168b761e859a538e74b36a2bcbb3450de293 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 31 Aug 2020 19:40:05 +0100 Subject: [PATCH 6/7] Feedback --- .../Core/Portable/IReferenceOrISignature.cs | 56 +++++++------------ 1 file changed, 21 insertions(+), 35 deletions(-) diff --git a/src/Compilers/Core/Portable/IReferenceOrISignature.cs b/src/Compilers/Core/Portable/IReferenceOrISignature.cs index 0ede4d03869fb..da1572985fcad 100644 --- a/src/Compilers/Core/Portable/IReferenceOrISignature.cs +++ b/src/Compilers/Core/Portable/IReferenceOrISignature.cs @@ -5,12 +5,17 @@ #nullable enable using System; -using System.Collections.Generic; +using System.Runtime.CompilerServices; using Microsoft.Cci; using Microsoft.CodeAnalysis.Symbols; namespace Microsoft.CodeAnalysis { + // These types are to enable fast-path devirtualization in the Jit. Dictionary, HashTable + // and ConcurrentDictionary will devirtualize (and potentially inline) the IEquatable.Equals + // method for a struct when the Comparer is unspecified in .NET Core, .NET 5; whereas specifying + // a Comparer will make .Equals and GetHashcode slower interface calls. + /// /// Used to devirtualize Dictionary/HashSet for EqualityComparer{T}.Default /// @@ -28,13 +33,16 @@ public IReferenceOrISignatureEquivalent(ISignature item) _item = item; } + // Needed to resolve ambiguity for types that implement both IReference and ISignature public IReferenceOrISignatureEquivalent(IMethodReference item) { _item = item; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] public bool Equals(IReferenceOrISignatureEquivalent other) { + // Fast inlinable ReferenceEquals object? x = _item; object? y = other._item; if (x is null) @@ -46,20 +54,12 @@ public bool Equals(IReferenceOrISignatureEquivalent other) return true; } - return Equals(x, y); + return EqualsSlow(x, y); } - private new static bool Equals(object? x, object? y) + private static bool EqualsSlow(object x, object? y) { - if (x is null) - { - return y is null; - } - else if (ReferenceEquals(x, y)) - { - return true; - } - else if (x is ISymbolInternal sx && y is ISymbolInternal sy) + if (x is ISymbolInternal sx && y is ISymbolInternal sy) { return sx.Equals(sy, TypeCompareKind.ConsiderEverything); } @@ -73,12 +73,7 @@ public bool Equals(IReferenceOrISignatureEquivalent other) } } - public override bool Equals(object? obj) - { - return obj is IReferenceOrISignatureEquivalent refOrSig ? - Equals(refOrSig) : - Equals(_item, obj); - } + public override bool Equals(object? obj) => false; public override int GetHashCode() => _item?.GetHashCode() ?? 0; @@ -94,37 +89,28 @@ public override bool Equals(object? obj) { private readonly object? _item; - public static implicit operator IReferenceOrISignature(IReferenceOrISignatureEquivalent d) => new IReferenceOrISignature(d.AsObject()); - - private IReferenceOrISignature(object? item) + public IReferenceOrISignature(IReference item) { _item = item; } - public IReferenceOrISignature(IReference item) + public IReferenceOrISignature(ISignature item) { _item = item; } - public IReferenceOrISignature(ISignature item) + // Used by implicit conversion + private IReferenceOrISignature(object? item) { _item = item; } - public bool Equals(IReferenceOrISignature other) - { - object? x = _item; - object? y = other._item; + public static implicit operator IReferenceOrISignature(IReferenceOrISignatureEquivalent item) + => new IReferenceOrISignature(item.AsObject()); - return ReferenceEquals(x, y); - } + public bool Equals(IReferenceOrISignature other) => ReferenceEquals(_item, other._item); - public override bool Equals(object? obj) - { - return obj is IReferenceOrISignature refOrSig ? - Equals(refOrSig) : - ReferenceEquals(_item, obj); - } + public override bool Equals(object? obj) => false; public override int GetHashCode() => _item?.GetHashCode() ?? 0; From 0d53ed82c000c70e4380cbab7c446fd22dc75ed5 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 31 Aug 2020 20:30:36 +0100 Subject: [PATCH 7/7] Nullability changes --- .../Core/Portable/CodeGen/TokenMap.cs | 15 +++-- .../Core/Portable/IReferenceOrISignature.cs | 56 ++++++------------- 2 files changed, 28 insertions(+), 43 deletions(-) diff --git a/src/Compilers/Core/Portable/CodeGen/TokenMap.cs b/src/Compilers/Core/Portable/CodeGen/TokenMap.cs index 1f9f102941800..fecdfeffa91da 100644 --- a/src/Compilers/Core/Portable/CodeGen/TokenMap.cs +++ b/src/Compilers/Core/Portable/CodeGen/TokenMap.cs @@ -7,6 +7,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.Threading; using Microsoft.Cci; @@ -28,9 +29,7 @@ internal sealed class TokenMap private object[] _items = Array.Empty(); private int _count = 0; - internal TokenMap() - { - } + internal TokenMap() { } public uint GetOrAddTokenFor(IReference item, out bool referenceAdded) { @@ -95,6 +94,10 @@ private uint AddItem(IReferenceOrISignatureEquivalent item, out bool referenceAd public object GetItem(uint token) { + // If a token has been handed out, then it should be always within _count of the + // current array and a lock is not required. + Debug.Assert(token < (uint)_count && _count <= _items.Length); + return _items[(int)token]; } @@ -102,11 +105,15 @@ public object GetItem(uint token) // should probably return ROA instead of IE and cache that in Module. (and no need to return count) public ReadOnlySpan GetAllItems() { + // Read _count before _items reference, to match inverse of the writes in AddItem. + // So _items is guaranteed to have at least count items; and a lock is not required. + // Read the count prior to getting the array int count = Volatile.Read(ref _count); + // Read the array reference object[] items = Volatile.Read(ref _items); - // Return a right sized copy of the array + // Return a right sized view of the array based on read count and reference. return new ReadOnlySpan(items, 0, count); } } diff --git a/src/Compilers/Core/Portable/IReferenceOrISignature.cs b/src/Compilers/Core/Portable/IReferenceOrISignature.cs index da1572985fcad..a83a17ec968b4 100644 --- a/src/Compilers/Core/Portable/IReferenceOrISignature.cs +++ b/src/Compilers/Core/Portable/IReferenceOrISignature.cs @@ -21,35 +21,22 @@ namespace Microsoft.CodeAnalysis /// internal readonly struct IReferenceOrISignatureEquivalent : IEquatable { - private readonly object? _item; + private readonly object _item; - public IReferenceOrISignatureEquivalent(IReference item) - { - _item = item; - } + public IReferenceOrISignatureEquivalent(IReference item) => _item = item; - public IReferenceOrISignatureEquivalent(ISignature item) - { - _item = item; - } + public IReferenceOrISignatureEquivalent(ISignature item) => _item = item; // Needed to resolve ambiguity for types that implement both IReference and ISignature - public IReferenceOrISignatureEquivalent(IMethodReference item) - { - _item = item; - } + public IReferenceOrISignatureEquivalent(IMethodReference item) => _item = item; [MethodImpl(MethodImplOptions.AggressiveInlining)] public bool Equals(IReferenceOrISignatureEquivalent other) { // Fast inlinable ReferenceEquals - object? x = _item; - object? y = other._item; - if (x is null) - { - return y is null; - } - else if (ReferenceEquals(x, y)) + var x = _item; + var y = other._item; + if (ReferenceEquals(x, y)) { return true; } @@ -57,7 +44,7 @@ public bool Equals(IReferenceOrISignatureEquivalent other) return EqualsSlow(x, y); } - private static bool EqualsSlow(object x, object? y) + private static bool EqualsSlow(object x, object y) { if (x is ISymbolInternal sx && y is ISymbolInternal sy) { @@ -75,11 +62,11 @@ private static bool EqualsSlow(object x, object? y) public override bool Equals(object? obj) => false; - public override int GetHashCode() => _item?.GetHashCode() ?? 0; + public override int GetHashCode() => _item.GetHashCode(); - public override string ToString() => _item?.ToString() ?? "null"; + public override string ToString() => _item.ToString() ?? "null"; - internal object AsObject() => _item!; + internal object AsObject() => _item; } /// @@ -87,23 +74,14 @@ private static bool EqualsSlow(object x, object? y) /// internal readonly struct IReferenceOrISignature : IEquatable { - private readonly object? _item; + private readonly object _item; - public IReferenceOrISignature(IReference item) - { - _item = item; - } + public IReferenceOrISignature(IReference item) => _item = item; - public IReferenceOrISignature(ISignature item) - { - _item = item; - } + public IReferenceOrISignature(ISignature item) => _item = item; // Used by implicit conversion - private IReferenceOrISignature(object? item) - { - _item = item; - } + private IReferenceOrISignature(object item) => _item = item; public static implicit operator IReferenceOrISignature(IReferenceOrISignatureEquivalent item) => new IReferenceOrISignature(item.AsObject()); @@ -112,8 +90,8 @@ public static implicit operator IReferenceOrISignature(IReferenceOrISignatureEqu public override bool Equals(object? obj) => false; - public override int GetHashCode() => _item?.GetHashCode() ?? 0; + public override int GetHashCode() => _item.GetHashCode(); - public override string ToString() => _item?.ToString() ?? "null"; + public override string ToString() => _item.ToString() ?? "null"; } }