diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/AllocFreeConcurrentStack.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/AllocFreeConcurrentStack.cs index b1550424afc7..ea0a5985cd57 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/AllocFreeConcurrentStack.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/AllocFreeConcurrentStack.cs @@ -9,31 +9,37 @@ namespace System.Collections.Immutable { - [DebuggerDisplay("Count = {stack.Count}")] - internal class AllocFreeConcurrentStack + [DebuggerDisplay("Count = {stack != null ? stack.Count : 0}")] + internal static class AllocFreeConcurrentStack { - /// - /// We use locks to protect this rather than ThreadLocal{T} because in perf tests - /// uncontested locks are much faster than looking up thread-local storage. - /// - private readonly Stack> stack = new Stack>(); + private const int MaxSize = 35; - public void TryAdd(T item) + [ThreadStatic] + private static Stack> stack; + + public static void TryAdd(T item) { - lock (this.stack) + if (stack == null) + { + stack = new Stack>(MaxSize); + } + + // Just in case we're in a scenario where an object is continually requested on one thread + // and returned on another, avoid unbounded growth of the stack. + if (stack.Count < MaxSize) { - this.stack.Push(new RefAsValueType(item)); + stack.Push(new RefAsValueType(item)); } } - public bool TryTake(out T item) + public static bool TryTake(out T item) { - lock (this.stack) + if (stack != null) { - int count = this.stack.Count; + int count = stack.Count; if (count > 0) { - item = this.stack.Pop().Value; + item = stack.Pop().Value; return true; } } diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList`1.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList`1.cs index 7747d4c2dc9e..8c4ea0ce38e4 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList`1.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList`1.cs @@ -1505,7 +1505,7 @@ public struct Enumerator : IEnumerator, ISecurePooledObjectUser /// A unique ID for this instance of this enumerator. /// Used to protect pooled objects from use after they are recycled. /// - private readonly Guid poolUserId; + private readonly int poolUserId; /// /// The starting index of the collection at which to begin enumeration. @@ -1571,25 +1571,21 @@ internal Enumerator(IBinaryTree root, Builder builder = null, int startIndex this.remainingCount = this.count; this.reversed = reversed; this.enumeratingBuilderVersion = builder != null ? builder.Version : -1; - this.poolUserId = Guid.NewGuid(); + this.poolUserId = SecureObjectPool.NewId(); + this.stack = null; if (this.count > 0) { - this.stack = null; if (!EnumeratingStacks.TryTake(this, out this.stack)) { this.stack = EnumeratingStacks.PrepNew(this, new Stack>>(root.Height)); } } - else - { - this.stack = null; - } this.Reset(); } /// - Guid ISecurePooledObjectUser.PoolUserId + int ISecurePooledObjectUser.PoolUserId { get { return this.poolUserId; } } diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedDictionary`2.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedDictionary`2.cs index 28c8839231d4..38f28e548b25 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedDictionary`2.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedDictionary`2.cs @@ -957,7 +957,7 @@ public struct Enumerator : IEnumerator>, ISecurePoole /// A unique ID for this instance of this enumerator. /// Used to protect pooled objects from use after they are recycled. /// - private readonly Guid poolUserId; + private readonly int poolUserId; /// /// The set being enumerated. @@ -992,11 +992,14 @@ internal Enumerator(IBinaryTree> root, Builder builde this.builder = builder; this.current = null; this.enumeratingBuilderVersion = builder != null ? builder.Version : -1; - this.poolUserId = Guid.NewGuid(); + this.poolUserId = SecureObjectPool.NewId(); this.stack = null; - if (!enumeratingStacks.TryTake(this, out this.stack)) + if (!this.root.IsEmpty) { - this.stack = enumeratingStacks.PrepNew(this, new Stack>>>(root.Height)); + if (!enumeratingStacks.TryTake(this, out this.stack)) + { + this.stack = enumeratingStacks.PrepNew(this, new Stack>>>(root.Height)); + } } this.Reset(); @@ -1020,7 +1023,7 @@ public KeyValuePair Current } /// - Guid ISecurePooledObjectUser.PoolUserId + int ISecurePooledObjectUser.PoolUserId { get { return this.poolUserId; } } @@ -1062,21 +1065,22 @@ public bool MoveNext() this.ThrowIfDisposed(); this.ThrowIfChanged(); - using (var stack = this.stack.Use(this)) + if (this.stack != null) { - if (stack.Value.Count > 0) - { - IBinaryTree> n = stack.Value.Pop().Value; - this.current = n; - this.PushLeft(n.Right); - return true; - } - else + using (var stack = this.stack.Use(this)) { - this.current = null; - return false; + if (stack.Value.Count > 0) + { + IBinaryTree> n = stack.Value.Pop().Value; + this.current = n; + this.PushLeft(n.Right); + return true; + } } } + + this.current = null; + return false; } /// @@ -1088,12 +1092,15 @@ public void Reset() this.enumeratingBuilderVersion = builder != null ? builder.Version : -1; this.current = null; - using (var stack = this.stack.Use(this)) + if (this.stack != null) { - stack.Value.Clear(); - } + using (var stack = this.stack.Use(this)) + { + stack.Value.Clear(); + } - this.PushLeft(this.root); + this.PushLeft(this.root); + } } /// diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet`1.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet`1.cs index 70cd88a2ad7a..b0ca16c69eab 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet`1.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet`1.cs @@ -1131,7 +1131,7 @@ public struct Enumerator : IEnumerator, ISecurePooledObjectUser /// A unique ID for this instance of this enumerator. /// Used to protect pooled objects from use after they are recycled. /// - private readonly Guid poolUserId; + private readonly int poolUserId; /// /// A flag indicating whether this enumerator works in reverse sort order. @@ -1181,7 +1181,7 @@ internal Enumerator(IBinaryTree root, Builder builder = null, bool reverse = this.current = null; this.reverse = reverse; this.enumeratingBuilderVersion = builder != null ? builder.Version : -1; - this.poolUserId = Guid.NewGuid(); + this.poolUserId = SecureObjectPool.NewId(); this.stack = null; if (!enumeratingStacks.TryTake(this, out this.stack)) { @@ -1192,7 +1192,7 @@ internal Enumerator(IBinaryTree root, Builder builder = null, bool reverse = } /// - Guid ISecurePooledObjectUser.PoolUserId + int ISecurePooledObjectUser.PoolUserId { get { return this.poolUserId; } } diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs index 08fca6dd776e..a7d57f572c0d 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs @@ -11,27 +11,53 @@ namespace System.Collections.Immutable { + /// + /// Object pooling utilities. + /// + internal class SecureObjectPool + { + /// + /// The ever-incrementing (and wrap-on-overflow) integer for owner id's. + /// + private static int poolUserIdCounter; + + /// + /// The ID reserved for unassigned objects. + /// + internal const int UnassignedId = -1; + + /// + /// Returns a new ID. + /// + internal static int NewId() + { + int result; + do + { + result = Interlocked.Increment(ref poolUserIdCounter); + } + while (result == UnassignedId); + + return result; + } + } + internal class SecureObjectPool where TCaller : ISecurePooledObjectUser { - private AllocFreeConcurrentStack> pool = new AllocFreeConcurrentStack>(); - public void TryAdd(TCaller caller, SecurePooledObject item) { - lock (item) + // Only allow the caller to recycle this object if it is the current owner. + if (caller.PoolUserId == item.Owner) { - // Only allow the caller to recycle this object if it is the current owner. - if (caller.PoolUserId == item.Owner) - { - item.Owner = Guid.Empty; - this.pool.TryAdd(item); - } + item.Owner = SecureObjectPool.UnassignedId; + AllocFreeConcurrentStack>.TryAdd(item); } } public bool TryTake(TCaller caller, out SecurePooledObject item) { - if (caller.PoolUserId != Guid.Empty && this.pool.TryTake(out item)) + if (caller.PoolUserId != SecureObjectPool.UnassignedId && AllocFreeConcurrentStack>.TryTake(out item)) { item.Owner = caller.PoolUserId; return true; @@ -54,13 +80,13 @@ public SecurePooledObject PrepNew(TCaller caller, T newValue) internal interface ISecurePooledObjectUser { - Guid PoolUserId { get; } + int PoolUserId { get; } } internal class SecurePooledObject { private readonly T value; - private Guid owner; + private int owner; internal SecurePooledObject(T newValue) { @@ -68,23 +94,13 @@ internal SecurePooledObject(T newValue) this.value = newValue; } - internal Guid Owner + /// + /// Gets or sets the current owner of this recyclable object. + /// + internal int Owner { - get - { - lock (this) - { - return this.owner; - } - } - - set - { - lock (this) - { - this.owner = value; - } - } + get { return this.owner; } + set { this.owner = value; } } internal SecurePooledObjectUser Use(TCaller caller) @@ -110,7 +126,6 @@ internal struct SecurePooledObjectUser : IDisposable internal SecurePooledObjectUser(SecurePooledObject value) { this.value = value; - Monitor.Enter(value); } internal T Value @@ -120,7 +135,6 @@ internal T Value public void Dispose() { - Monitor.Exit(value); } } } diff --git a/src/System.Collections.Immutable/tests/ImmutableDictionaryTest.cs b/src/System.Collections.Immutable/tests/ImmutableDictionaryTest.cs index 0ed94bce227d..14abf4cedd37 100644 --- a/src/System.Collections.Immutable/tests/ImmutableDictionaryTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableDictionaryTest.cs @@ -298,9 +298,10 @@ public void GetValueOrDefaultOfConcreteType() [Fact] public void EnumeratorRecyclingMisuse() { - var collection = ImmutableDictionary.Create(); + var collection = ImmutableDictionary.Create().Add(5, 3); var enumerator = collection.GetEnumerator(); var enumeratorCopy = enumerator; + Assert.True(enumerator.MoveNext()); Assert.False(enumerator.MoveNext()); enumerator.Dispose(); Assert.Throws(() => enumerator.MoveNext()); @@ -315,6 +316,7 @@ public void EnumeratorRecyclingMisuse() // We expect that acquiring a new enumerator will use the same underlying Stack object, // but that it will not throw exceptions for the new enumerator. enumerator = collection.GetEnumerator(); + Assert.True(enumerator.MoveNext()); Assert.False(enumerator.MoveNext()); Assert.Throws(() => enumerator.Current); enumerator.Dispose(); diff --git a/src/System.Collections.Immutable/tests/ImmutableHashSetTest.cs b/src/System.Collections.Immutable/tests/ImmutableHashSetTest.cs index db4a16b6efc8..2d249d8a2ee8 100644 --- a/src/System.Collections.Immutable/tests/ImmutableHashSetTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableHashSetTest.cs @@ -73,9 +73,10 @@ public void EnumeratorWithHashCollisionsTest() [Fact] public void EnumeratorRecyclingMisuse() { - var collection = ImmutableHashSet.Create(); + var collection = ImmutableHashSet.Create().Add(5); var enumerator = collection.GetEnumerator(); var enumeratorCopy = enumerator; + Assert.True(enumerator.MoveNext()); Assert.False(enumerator.MoveNext()); enumerator.Dispose(); Assert.Throws(() => enumerator.MoveNext()); @@ -90,6 +91,7 @@ public void EnumeratorRecyclingMisuse() // We expect that acquiring a new enumerator will use the same underlying Stack object, // but that it will not throw exceptions for the new enumerator. enumerator = collection.GetEnumerator(); + Assert.True(enumerator.MoveNext()); Assert.False(enumerator.MoveNext()); Assert.Throws(() => enumerator.Current); enumerator.Dispose(); diff --git a/src/System.Collections.Immutable/tests/ImmutableSortedDictionaryTest.cs b/src/System.Collections.Immutable/tests/ImmutableSortedDictionaryTest.cs index 3462cbf6028b..c0423b098d50 100644 --- a/src/System.Collections.Immutable/tests/ImmutableSortedDictionaryTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableSortedDictionaryTest.cs @@ -3,6 +3,8 @@ using System; using System.Collections.Generic; +using System.Diagnostics; +using System.IO; using System.Linq; using System.Text; using Xunit; @@ -316,9 +318,10 @@ public void WithComparersEmptyCollection() [Fact] public void EnumeratorRecyclingMisuse() { - var collection = ImmutableSortedDictionary.Create(); + var collection = ImmutableSortedDictionary.Create().Add(3, 5); var enumerator = collection.GetEnumerator(); var enumeratorCopy = enumerator; + Assert.True(enumerator.MoveNext()); Assert.False(enumerator.MoveNext()); enumerator.Dispose(); Assert.Throws(() => enumerator.MoveNext()); @@ -334,11 +337,60 @@ public void EnumeratorRecyclingMisuse() // We expect that acquiring a new enumerator will use the same underlying Stack object, // but that it will not throw exceptions for the new enumerator. enumerator = collection.GetEnumerator(); + Assert.True(enumerator.MoveNext()); Assert.False(enumerator.MoveNext()); Assert.Throws(() => enumerator.Current); enumerator.Dispose(); } + ////[Fact] // not really a functional test -- but very useful to enable when collecting perf traces. + public void EnumerationPerformance() + { + var dictionary = Enumerable.Range(1, 1000).ToImmutableSortedDictionary(k => k, k => k); + + var timing = new TimeSpan[3]; + var sw = new Stopwatch(); + for (int j = 0; j < timing.Length; j++) + { + sw.Start(); + for (int i = 0; i < 10000; i++) + { + foreach (var entry in dictionary) + { + } + } + + timing[j] = sw.Elapsed; + sw.Reset(); + } + + File.AppendAllText(Environment.ExpandEnvironmentVariables(@"%TEMP%\timing.txt"), string.Join(Environment.NewLine, timing)); + } + + ////[Fact] // not really a functional test -- but very useful to enable when collecting perf traces. + public void EnumerationPerformance_Empty() + { + var dictionary = ImmutableSortedDictionary.Empty; + + var timing = new TimeSpan[3]; + var sw = new Stopwatch(); + for (int j = 0; j < timing.Length; j++) + { + sw.Start(); + for (int i = 0; i < 10000; i++) + { + foreach (var entry in dictionary) + { + } + } + + timing[j] = sw.Elapsed; + sw.Reset(); + } + + File.AppendAllText(Environment.ExpandEnvironmentVariables(@"%TEMP%\timing_empty.txt"), string.Join(Environment.NewLine, timing)); + } + protected override IImmutableDictionary Empty() { return ImmutableSortedDictionaryTest.Empty();