From a75a1339fdb9f9af5daf3ff11dc7e1949c0713f2 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Thu, 6 Nov 2014 08:40:27 -0800 Subject: [PATCH 1/8] Improve performance of enumerating empty collections. Avoid perf hit from acquiring a reusable stack object to enumerate a collection that is empty, and thus requires no stack object. This improves ImmutableSortedDictionary directly, and both ImmutableDictionary and ImmutableHashSet benefit as well since they use ImmutableSortedDictionary internally. --- .../Collections/Immutable/ImmutableList`1.cs | 6 +-- .../Immutable/ImmutableSortedDictionary`2.cs | 41 +++++++++++-------- 2 files changed, 25 insertions(+), 22 deletions(-) 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..eff3ae10b23a 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 @@ -1572,18 +1572,14 @@ internal Enumerator(IBinaryTree root, Builder builder = null, int startIndex this.reversed = reversed; this.enumeratingBuilderVersion = builder != null ? builder.Version : -1; this.poolUserId = Guid.NewGuid(); + 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(); } 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..f0ecc2a78166 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 @@ -994,9 +994,12 @@ internal Enumerator(IBinaryTree> root, Builder builde this.enumeratingBuilderVersion = builder != null ? builder.Version : -1; this.poolUserId = Guid.NewGuid(); 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(); @@ -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); + } } /// From 11eedf76c1f5de447a8ac5ca092d75169438099b Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Tue, 11 Nov 2014 10:56:01 -0800 Subject: [PATCH 2/8] Fix tests to pass after perf improvement. --- .../tests/ImmutableDictionaryTest.cs | 4 +- .../tests/ImmutableHashSetTest.cs | 4 +- .../tests/ImmutableSortedDictionaryTest.cs | 54 ++++++++++++++++++- 3 files changed, 59 insertions(+), 3 deletions(-) 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..4f5b10c96f1f 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] + 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(); From ab1e10fe67d79077724de8346a6a06101abb3192 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Tue, 11 Nov 2014 11:02:25 -0800 Subject: [PATCH 3/8] Exclude method used for perf trace collection from functional tests. --- .../tests/ImmutableSortedDictionaryTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Collections.Immutable/tests/ImmutableSortedDictionaryTest.cs b/src/System.Collections.Immutable/tests/ImmutableSortedDictionaryTest.cs index 4f5b10c96f1f..c0423b098d50 100644 --- a/src/System.Collections.Immutable/tests/ImmutableSortedDictionaryTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableSortedDictionaryTest.cs @@ -367,7 +367,7 @@ public void EnumerationPerformance() File.AppendAllText(Environment.ExpandEnvironmentVariables(@"%TEMP%\timing.txt"), string.Join(Environment.NewLine, timing)); } - [Fact] + ////[Fact] // not really a functional test -- but very useful to enable when collecting perf traces. public void EnumerationPerformance_Empty() { var dictionary = ImmutableSortedDictionary.Empty; From 7f84859e0dae8c3011c997f920a7121c71c38b27 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Thu, 6 Nov 2014 08:43:00 -0800 Subject: [PATCH 4/8] Switches object pool IDs from GUIDs to Int32. This cuts out the expensive Guid.op_Equality step from ThrowDisposedIfNotOwned. --- .../Collections/Immutable/ImmutableList`1.cs | 6 +-- .../Immutable/ImmutableSortedDictionary`2.cs | 6 +-- .../Immutable/ImmutableSortedSet`1.cs | 6 +-- .../Collections/Immutable/SecureObjectPool.cs | 40 ++++++++++++++++--- 4 files changed, 44 insertions(+), 14 deletions(-) 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 eff3ae10b23a..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,7 +1571,7 @@ 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) { @@ -1585,7 +1585,7 @@ internal Enumerator(IBinaryTree root, Builder builder = null, int startIndex } /// - 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 f0ecc2a78166..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,7 +992,7 @@ 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 (!this.root.IsEmpty) { @@ -1023,7 +1023,7 @@ public KeyValuePair Current } /// - Guid ISecurePooledObjectUser.PoolUserId + int ISecurePooledObjectUser.PoolUserId { get { return this.poolUserId; } } 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..fdbd8a09acc6 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs @@ -11,6 +11,36 @@ 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 = Interlocked.Increment(ref poolUserIdCounter); + if (result == UnassignedId) + { + result = Interlocked.Increment(ref poolUserIdCounter); + } + + return result; + } + } + internal class SecureObjectPool where TCaller : ISecurePooledObjectUser { @@ -23,7 +53,7 @@ public void TryAdd(TCaller caller, SecurePooledObject item) // Only allow the caller to recycle this object if it is the current owner. if (caller.PoolUserId == item.Owner) { - item.Owner = Guid.Empty; + item.Owner = SecureObjectPool.UnassignedId; this.pool.TryAdd(item); } } @@ -31,7 +61,7 @@ public void TryAdd(TCaller caller, SecurePooledObject item) public bool TryTake(TCaller caller, out SecurePooledObject item) { - if (caller.PoolUserId != Guid.Empty && this.pool.TryTake(out item)) + if (caller.PoolUserId != SecureObjectPool.UnassignedId && this.pool.TryTake(out item)) { item.Owner = caller.PoolUserId; return true; @@ -54,13 +84,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,7 +98,7 @@ internal SecurePooledObject(T newValue) this.value = newValue; } - internal Guid Owner + internal int Owner { get { From fb19533a1534b69d06656c1dd20021ca08c26e51 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Thu, 6 Nov 2014 08:43:22 -0800 Subject: [PATCH 5/8] Add code comment. --- .../src/System/Collections/Immutable/SecureObjectPool.cs | 8 ++++++++ 1 file changed, 8 insertions(+) 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 fdbd8a09acc6..35eee24d0d3b 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs @@ -98,6 +98,14 @@ internal SecurePooledObject(T newValue) this.value = newValue; } + /// + /// Gets or sets the current owner of this recyclable object. + /// + /// + /// We lock (this) because SecurePooledObjectUser calls also locks this (through Monitor.Enter) + /// and this ensure we don't allow reassignment of the owner while the current owner is using it. + /// We also lock (this) in TryAdd. + /// internal int Owner { get From b460c2701270b4c8ff8e6aeebd8a4071b283ceda Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Thu, 6 Nov 2014 08:43:45 -0800 Subject: [PATCH 6/8] Switch from locks to ThreadStatic for AllocFreeConcurrentStack. --- .../Immutable/AllocFreeConcurrentStack.cs | 34 +++++++++++-------- .../Collections/Immutable/SecureObjectPool.cs | 6 ++-- 2 files changed, 22 insertions(+), 18 deletions(-) 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/SecureObjectPool.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs index 35eee24d0d3b..08cdf15c5994 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs @@ -44,8 +44,6 @@ internal static int NewId() internal class SecureObjectPool where TCaller : ISecurePooledObjectUser { - private AllocFreeConcurrentStack> pool = new AllocFreeConcurrentStack>(); - public void TryAdd(TCaller caller, SecurePooledObject item) { lock (item) @@ -54,14 +52,14 @@ public void TryAdd(TCaller caller, SecurePooledObject item) if (caller.PoolUserId == item.Owner) { item.Owner = SecureObjectPool.UnassignedId; - this.pool.TryAdd(item); + AllocFreeConcurrentStack>.TryAdd(item); } } } public bool TryTake(TCaller caller, out SecurePooledObject item) { - if (caller.PoolUserId != SecureObjectPool.UnassignedId && this.pool.TryTake(out item)) + if (caller.PoolUserId != SecureObjectPool.UnassignedId && AllocFreeConcurrentStack>.TryTake(out item)) { item.Owner = caller.PoolUserId; return true; From f3f9e2069c0bcd2ef41217ce1c42255c66243c2d Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Thu, 6 Nov 2014 08:44:13 -0800 Subject: [PATCH 7/8] Remove use of locks that significantly slow down enumeration. Enumerating some immutable collections are necessarily slower than ordinary collections because they have to walk binary trees. But we needn't throw in locks into the enumerator and slow it down further in an attempt to claim enumerators are somehow thread-safe. They're not. And misusing them across threads concurrently would already lead to corruption. By removing the locks here, we've significantly improved performance of enumerating ImmutableSortedDictionary (and the other binary tree based collections). Even though the locks weren't contested, they still took a significant chunk of time. The loss of the locking means that technically if the enumerator is in use across multiple threads then now it's possible for a recycled Stack to still be in use on another thread, leading to data corruption for the possibly innocent subsequent user of that stack. Since we've already decided that immutable collections will not be considered as behind a security boundary (i.e. we won't treat such possibilities as security compromising to the core system), this seems quite reasonable for such an unlikely and unsupported pattern to carry this risk given the huge wins it has for all the supported cases. --- .../Collections/Immutable/SecureObjectPool.cs | 27 ++----------------- 1 file changed, 2 insertions(+), 25 deletions(-) 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 08cdf15c5994..92ba51c27d1b 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs @@ -46,15 +46,12 @@ internal class SecureObjectPool { 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) { item.Owner = SecureObjectPool.UnassignedId; AllocFreeConcurrentStack>.TryAdd(item); } - } } public bool TryTake(TCaller caller, out SecurePooledObject item) @@ -99,28 +96,10 @@ internal SecurePooledObject(T newValue) /// /// Gets or sets the current owner of this recyclable object. /// - /// - /// We lock (this) because SecurePooledObjectUser calls also locks this (through Monitor.Enter) - /// and this ensure we don't allow reassignment of the owner while the current owner is using it. - /// We also lock (this) in TryAdd. - /// 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) @@ -146,7 +125,6 @@ internal struct SecurePooledObjectUser : IDisposable internal SecurePooledObjectUser(SecurePooledObject value) { this.value = value; - Monitor.Enter(value); } internal T Value @@ -156,7 +134,6 @@ internal T Value public void Dispose() { - Monitor.Exit(value); } } } From 64929227e32f6526371acecb5a7b37f5ecf52e3e Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Tue, 11 Nov 2014 10:50:05 -0800 Subject: [PATCH 8/8] Defend against pathalogical thread-safety case. If SecureObjectPool.NewId were to execute in a concurrent scenario where one thread actually was suspended while other threads were allowed to increment the counter enough to go through all 32-bits of values and the timing happened to be _just write_, we could theoretically return UnassignedId. This fixes it by looping until we get it right. --- .../Collections/Immutable/SecureObjectPool.cs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) 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 92ba51c27d1b..a7d57f572c0d 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/SecureObjectPool.cs @@ -31,11 +31,12 @@ internal class SecureObjectPool /// internal static int NewId() { - int result = Interlocked.Increment(ref poolUserIdCounter); - if (result == UnassignedId) + int result; + do { result = Interlocked.Increment(ref poolUserIdCounter); } + while (result == UnassignedId); return result; } @@ -46,12 +47,12 @@ internal class SecureObjectPool { public void TryAdd(TCaller caller, SecurePooledObject item) { - // Only allow the caller to recycle this object if it is the current owner. - if (caller.PoolUserId == item.Owner) - { - item.Owner = SecureObjectPool.UnassignedId; - AllocFreeConcurrentStack>.TryAdd(item); - } + // Only allow the caller to recycle this object if it is the current owner. + if (caller.PoolUserId == item.Owner) + { + item.Owner = SecureObjectPool.UnassignedId; + AllocFreeConcurrentStack>.TryAdd(item); + } } public bool TryTake(TCaller caller, out SecurePooledObject item)