From 4578f8222b5ec36357dd67503ed537d055bb3f17 Mon Sep 17 00:00:00 2001 From: CyrusNajmabadi Date: Wed, 11 May 2016 12:50:30 -0700 Subject: [PATCH 1/5] Improve OrderPreservingMultiDictionary. 1. Support allocation-free enumeration of the dictionary. 2. Pool internal data so that less garbage is churned when creating and releasing dictionaries. --- .../OrderPreservingMultiDictionary.cs | 269 +++++++++++------- 1 file changed, 169 insertions(+), 100 deletions(-) diff --git a/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs b/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs index a7f32443622e9..52be50b18b713 100644 --- a/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs +++ b/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs @@ -1,5 +1,7 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; +using System.Collections; using System.Collections.Generic; using System.Collections.Immutable; using System.Diagnostics; @@ -15,7 +17,8 @@ namespace Microsoft.CodeAnalysis.Collections /// /// Always uses the default comparer. /// - internal sealed class OrderPreservingMultiDictionary + internal sealed class OrderPreservingMultiDictionary : + IEnumerable.ValueSet>> { #region Pooling @@ -30,6 +33,11 @@ public void Free() { if (_dictionary != null) { + foreach (var kvp in _dictionary) + { + kvp.Value.Free(); + } + _dictionary.Free(); _dictionary = null; } @@ -57,159 +65,220 @@ public void Free() #endregion Pooling + private static readonly Dictionary s_emptyDictionary = new Dictionary(); + + private PooledDictionary _dictionary; + public OrderPreservingMultiDictionary() { } - // store either a single V or an ArrayBuilder - /// - /// Each value is either a single V or an . - /// Null when the dictionary is empty. - /// Don't access the field directly. - /// - private PooledDictionary _dictionary; - private void EnsureDictionary() { - _dictionary = _dictionary ?? PooledDictionary.GetInstance(); + _dictionary = _dictionary ?? PooledDictionary.GetInstance(); } - public bool IsEmpty - { - get { return _dictionary == null; } - } + public bool IsEmpty => _dictionary == null; /// /// Add a value to the dictionary. /// public void Add(K k, V v) { - object item; + ValueSet item; if (!this.IsEmpty && _dictionary.TryGetValue(k, out item)) { - var arrayBuilder = item as ArrayBuilder; - if (arrayBuilder == null) - { - // Promote from singleton V to ArrayBuilder. - Debug.Assert(item is V, "Item must be either a V or an ArrayBuilder"); - arrayBuilder = new ArrayBuilder(2); - arrayBuilder.Add((V)item); - arrayBuilder.Add(v); - _dictionary[k] = arrayBuilder; - } - else - { - arrayBuilder.Add(v); - } + _dictionary[k] = item.Add(v); } else { this.EnsureDictionary(); - _dictionary[k] = v; + _dictionary[k] = new ValueSet(v); } } - /// - /// Add multiple values to the dictionary. - /// - public void AddRange(K k, ImmutableArray values) + public Dictionary.Enumerator GetEnumerator() { - if (values.IsEmpty) - return; - - object item; - ArrayBuilder arrayBuilder; + return IsEmpty ? s_emptyDictionary.GetEnumerator() : _dictionary.GetEnumerator(); + } - if (!this.IsEmpty && _dictionary.TryGetValue(k, out item)) - { - arrayBuilder = item as ArrayBuilder; - if (arrayBuilder == null) - { - // Promote from singleton V to ArrayBuilder. - Debug.Assert(item is V, "Item must be either a V or an ArrayBuilder"); - arrayBuilder = new ArrayBuilder(1 + values.Length); - arrayBuilder.Add((V)item); - arrayBuilder.AddRange(values); - _dictionary[k] = arrayBuilder; - } - else - { - arrayBuilder.AddRange(values); - } - } - else - { - this.EnsureDictionary(); + IEnumerator> IEnumerable>.GetEnumerator() + { + return GetEnumerator(); + } - if (values.Length == 1) - { - _dictionary[k] = values[0]; - } - else - { - arrayBuilder = new ArrayBuilder(values.Length); - arrayBuilder.AddRange(values); - _dictionary[k] = arrayBuilder; - } - } + IEnumerator IEnumerable.GetEnumerator() + { + return GetEnumerator(); } /// - /// Get the number of values associated with a key. + /// Get all values associated with K, in the order they were added. + /// Returns empty read-only array if no values were present. /// - public int GetCountForKey(K k) + public ImmutableArray this[K k] { - object item; - if (!this.IsEmpty && _dictionary.TryGetValue(k, out item)) + get { - return (item as ArrayBuilder)?.Count ?? 1; - } + ValueSet valueSet; + if (!this.IsEmpty && _dictionary.TryGetValue(k, out valueSet)) + { + return valueSet.Items; + } - return 0; + return ImmutableArray.Empty; + } } /// - /// Returns true if one or more items with given key have been added. + /// Get a collection of all the keys. /// - public bool ContainsKey(K k) + public Dictionary.KeyCollection Keys { - return !this.IsEmpty && _dictionary.ContainsKey(k); + get { return this.IsEmpty ? s_emptyDictionary.Keys : _dictionary.Keys; } } - /// - /// Get all values associated with K, in the order they were added. - /// Returns empty read-only array if no values were present. - /// - public ImmutableArray this[K k] + public struct ValueSet : IEnumerable { - get + // store either a single V or an ArrayBuilder + /// + /// Each value is either a single V or an . + /// Null when the dictionary is empty. + /// Don't access the field directly. + /// + private readonly object _value; + private static ObjectPool> s_builderPool = new ObjectPool>( + () => new ArrayBuilder(size: 2)); + + internal ValueSet(V value) { - object item; - if (!this.IsEmpty && _dictionary.TryGetValue(k, out item)) + _value = value; + } + + internal ValueSet(ArrayBuilder values) + { + _value = values; + } + + internal void Free() + { + var arrayBuilder = _value as ArrayBuilder; + if (arrayBuilder != null) + { + arrayBuilder.Clear(); + s_builderPool.Free(arrayBuilder); + } + } + + internal V this[int index] + { + get { - var arrayBuilder = item as ArrayBuilder; + var arrayBuilder = _value as ArrayBuilder; + if (arrayBuilder == null) + { + if (index == 0) + { + return (V)_value; + } + else + { + throw new IndexOutOfRangeException(); + } + } + else + { + return arrayBuilder[index]; + } + } + } + + internal ImmutableArray Items + { + get + { + var arrayBuilder = _value as ArrayBuilder; if (arrayBuilder == null) { // promote singleton to set - Debug.Assert(item is V, "Item must be either a V or an ArrayBuilder"); - return ImmutableArray.Create((V)item); + Debug.Assert(_value is V, "Item must be a a V"); + return ImmutableArray.Create((V)_value); } else { return arrayBuilder.ToImmutable(); } } + } - return ImmutableArray.Empty; + internal int Count => (_value as ArrayBuilder)?.Count ?? 1; + + IEnumerator IEnumerable.GetEnumerator() + { + return GetEnumerator(); } - } - /// - /// Get a collection of all the keys. - /// - public ICollection Keys - { - get { return this.IsEmpty ? SpecializedCollections.EmptyCollection() : _dictionary.Keys; } + IEnumerator IEnumerable.GetEnumerator() + { + return GetEnumerator(); + } + + public Enumerator GetEnumerator() + { + return new Enumerator(this); + } + + internal ValueSet Add(V item) + { + var arrayBuilder = _value as ArrayBuilder; + if (arrayBuilder == null) + { + // Promote from singleton V to ArrayBuilder. + Debug.Assert(_value is V, "_value must be a V"); + arrayBuilder = s_builderPool.Allocate(); + arrayBuilder.Add((V)_value); + arrayBuilder.Add(item); + } + else + { + arrayBuilder.Add(item); + } + + return new ValueSet(arrayBuilder); + } + + public struct Enumerator : IEnumerator + { + private readonly ValueSet _valueSet; + private readonly int _count; + private int _index; + + public Enumerator(ValueSet valueSet) + { + _valueSet = valueSet; + _count = _valueSet.Count; + _index = -1; + } + + public V Current => _valueSet[_index]; + + object IEnumerator.Current => Current; + + public bool MoveNext() + { + _index++; + return _index < _count; + } + + public void Reset() + { + _index = -1; + } + + public void Dispose() + { + } + } } } -} +} \ No newline at end of file From 0863ef985d2bfa1548273e0e22ba2f3009e055c8 Mon Sep 17 00:00:00 2001 From: CyrusNajmabadi Date: Wed, 11 May 2016 13:02:24 -0700 Subject: [PATCH 2/5] Move to using a multidictionary in our SymbolTreeInfo code. --- .../SymbolTree/SymbolTreeInfo_Metadata.cs | 31 ++++++------------- .../Core/Portable/Workspaces.csproj | 3 ++ 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Metadata.cs b/src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Metadata.cs index a7572892445d3..4f182b14d43f4 100644 --- a/src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Metadata.cs +++ b/src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Metadata.cs @@ -6,26 +6,13 @@ using System.Text; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Collections; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.FindSymbols { internal partial class SymbolTreeInfo { - private static SimplePool> s_definitionMapPool = - new SimplePool>(() => new MultiDictionary()); - - private static MultiDictionary AllocateDefinitionMap() - { - return s_definitionMapPool.Allocate(); - } - - private static void FreeDefinitionMap(MultiDictionary definitionMap) - { - definitionMap.Clear(); - s_definitionMapPool.Free(definitionMap); - } - /// /// this gives you SymbolTreeInfo for a metadata /// @@ -117,7 +104,7 @@ private static void GenerateMetadataNodes(MetadataReader metadataReader, List unsortedNodes) { - var definitionMap = AllocateDefinitionMap(); + var definitionMap = OrderPreservingMultiDictionary.GetInstance(); try { LookupMetadataDefinitions(reader, globalNamespace, definitionMap); @@ -132,7 +119,7 @@ private static void GenerateMetadataNodes(MetadataReader metadataReader, List.ValueSet definitionsWithSameName, + OrderPreservingMultiDictionary.ValueSet definitionsWithSameName, List unsortedNodes) { var node = new Node(name, parentIndex); @@ -148,7 +135,7 @@ private static void GenerateMetadataNodes(MetadataReader metadataReader, List.GetInstance(); try { foreach (var definition in definitionsWithSameName) @@ -166,13 +153,13 @@ private static void GenerateMetadataNodes(MetadataReader metadataReader, List definitionMap) + OrderPreservingMultiDictionary definitionMap) { switch (definition.Kind) { @@ -187,7 +174,7 @@ private static void GenerateMetadataNodes(MetadataReader metadataReader, List definitionMap) + OrderPreservingMultiDictionary definitionMap) { // Only bother looking for extension methods in static types. if ((typeDefinition.Attributes & TypeAttributes.Abstract) != 0 && @@ -235,7 +222,7 @@ private static void GenerateMetadataNodes(MetadataReader metadataReader, List definitionMap) + OrderPreservingMultiDictionary definitionMap) { foreach (var child in namespaceDefinition.NamespaceDefinitions) { diff --git a/src/Workspaces/Core/Portable/Workspaces.csproj b/src/Workspaces/Core/Portable/Workspaces.csproj index c81ead23270a7..9d0f61ec65b76 100644 --- a/src/Workspaces/Core/Portable/Workspaces.csproj +++ b/src/Workspaces/Core/Portable/Workspaces.csproj @@ -41,6 +41,9 @@ InternalUtilities\ImmutableArrayExtensions.cs + + Utilities\CompilerUtilities\OrderPreservingMultiDictionary.cs + Collections\PooledStringBuilder.cs From 253b5f8d2d2b8084291e45ea64782ffb4131d11b Mon Sep 17 00:00:00 2001 From: CyrusNajmabadi Date: Wed, 11 May 2016 13:10:45 -0700 Subject: [PATCH 3/5] Clean up code and add comments to clarify. --- .../OrderPreservingMultiDictionary.cs | 41 +++++++++++++------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs b/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs index 52be50b18b713..f73f6695a13f5 100644 --- a/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs +++ b/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs @@ -10,9 +10,8 @@ namespace Microsoft.CodeAnalysis.Collections { /// - /// A MultiDictionary that allows only adding, and - /// preserves the order of values added to the dictionary. - /// Thread-safe for reading, but not for adding. + /// A MultiDictionary that allows only adding, and preserves the order of values added to the + /// dictionary. Thread-safe for reading, but not for adding. /// /// /// Always uses the default comparer. @@ -33,6 +32,7 @@ public void Free() { if (_dictionary != null) { + // Allow our ValueSets to return their underlying ArrayBuilders to the pool. foreach (var kvp in _dictionary) { kvp.Value.Free(); @@ -65,8 +65,11 @@ public void Free() #endregion Pooling + // An empty dictionary we keep around to simplify certain operations (like "Keys") + // when we don't have an underlying dictionary of our own. private static readonly Dictionary s_emptyDictionary = new Dictionary(); + // The underlying dictionary we store our data in. null if we are empty. private PooledDictionary _dictionary; public OrderPreservingMultiDictionary() @@ -78,17 +81,20 @@ private void EnsureDictionary() _dictionary = _dictionary ?? PooledDictionary.GetInstance(); } - public bool IsEmpty => _dictionary == null; + public bool IsEmpty => _dictionary == null; /// /// Add a value to the dictionary. /// public void Add(K k, V v) { - ValueSet item; - if (!this.IsEmpty && _dictionary.TryGetValue(k, out item)) + ValueSet valueSet; + if (!this.IsEmpty && _dictionary.TryGetValue(k, out valueSet)) { - _dictionary[k] = item.Add(v); + Debug.Assert(valueSet.Count >= 1); + // Have to re-store the ValueSet in case we upgraded the existing ValueSet from + // holding a single item to holding multiple items. + _dictionary[k] = valueSet.WithAddedItem(v); } else { @@ -123,6 +129,7 @@ IEnumerator IEnumerable.GetEnumerator() ValueSet valueSet; if (!this.IsEmpty && _dictionary.TryGetValue(k, out valueSet)) { + Debug.Assert(valueSet.Count >= 1); return valueSet.Items; } @@ -140,16 +147,19 @@ IEnumerator IEnumerable.GetEnumerator() public struct ValueSet : IEnumerable { - // store either a single V or an ArrayBuilder /// /// Each value is either a single V or an . - /// Null when the dictionary is empty. - /// Don't access the field directly. + /// Never null. /// private readonly object _value; + + // By default we allocate array builders with a size of two. That's two store + // the single item already in _value, and to store the item we're adding. + // In general, we presume that the amount of values per key will be low, so this + // means we have very little overhead when there are multiple keys per value. private static ObjectPool> s_builderPool = new ObjectPool>( () => new ArrayBuilder(size: 2)); - + internal ValueSet(V value) { _value = value; @@ -174,6 +184,8 @@ internal void Free() { get { + Debug.Assert(this.Count >= 1); + var arrayBuilder = _value as ArrayBuilder; if (arrayBuilder == null) { @@ -197,6 +209,8 @@ internal ImmutableArray Items { get { + Debug.Assert(this.Count >= 1); + var arrayBuilder = _value as ArrayBuilder; if (arrayBuilder == null) { @@ -228,8 +242,10 @@ public Enumerator GetEnumerator() return new Enumerator(this); } - internal ValueSet Add(V item) + internal ValueSet WithAddedItem(V item) { + Debug.Assert(this.Count >= 1); + var arrayBuilder = _value as ArrayBuilder; if (arrayBuilder == null) { @@ -257,6 +273,7 @@ public Enumerator(ValueSet valueSet) { _valueSet = valueSet; _count = _valueSet.Count; + Debug.Assert(_count >= 1); _index = -1; } From de0888c13cbf54b0494ef3a5e98759536dfed3bd Mon Sep 17 00:00:00 2001 From: CyrusNajmabadi Date: Wed, 11 May 2016 14:29:43 -0700 Subject: [PATCH 4/5] Use the built-in pooling support already in ArrayBuilder. --- .../OrderPreservingMultiDictionary.cs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs b/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs index f73f6695a13f5..2c2a10a979a3b 100644 --- a/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs +++ b/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs @@ -153,13 +153,6 @@ public struct ValueSet : IEnumerable /// private readonly object _value; - // By default we allocate array builders with a size of two. That's two store - // the single item already in _value, and to store the item we're adding. - // In general, we presume that the amount of values per key will be low, so this - // means we have very little overhead when there are multiple keys per value. - private static ObjectPool> s_builderPool = new ObjectPool>( - () => new ArrayBuilder(size: 2)); - internal ValueSet(V value) { _value = value; @@ -175,8 +168,7 @@ internal void Free() var arrayBuilder = _value as ArrayBuilder; if (arrayBuilder != null) { - arrayBuilder.Clear(); - s_builderPool.Free(arrayBuilder); + arrayBuilder.Free(); } } @@ -251,7 +243,12 @@ internal ValueSet WithAddedItem(V item) { // Promote from singleton V to ArrayBuilder. Debug.Assert(_value is V, "_value must be a V"); - arrayBuilder = s_builderPool.Allocate(); + + // By default we allocate array builders with a size of two. That's to store + // the single item already in _value, and to store the item we're adding. + // In general, we presume that the amount of values per key will be low, so this + // means we have very little overhead when there are multiple keys per value. + arrayBuilder = ArrayBuilder.GetInstance(capacity: 2); arrayBuilder.Add((V)_value); arrayBuilder.Add(item); } From 20da7832b53bfdc732347f28f7be393092c86fec Mon Sep 17 00:00:00 2001 From: CyrusNajmabadi Date: Wed, 11 May 2016 14:33:00 -0700 Subject: [PATCH 5/5] Simplify code. --- .../Portable/Collections/OrderPreservingMultiDictionary.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs b/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs index 2c2a10a979a3b..40c444b592a8f 100644 --- a/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs +++ b/src/Compilers/Core/Portable/Collections/OrderPreservingMultiDictionary.cs @@ -166,10 +166,7 @@ internal ValueSet(ArrayBuilder values) internal void Free() { var arrayBuilder = _value as ArrayBuilder; - if (arrayBuilder != null) - { - arrayBuilder.Free(); - } + arrayBuilder?.Free(); } internal V this[int index]