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); + } } /// 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();