Merge pull request #814 from LilithSilver/fix-bag-enumerator

Merged

src/cs/MonoGame.Extended/Collections/Bag.cs

@@ -153,40 +153,63 @@ namespace MonoGame.Extended.Collections
             Array.Copy(oldElements, 0, _items, 0, oldElements.Length);
         }
 
-        IEnumerator<T> IEnumerable<T>.GetEnumerator()
+        IEnumerator<T> IEnumerable<T>.GetEnumerator() => GetEnumerator();
+
+        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+
+        /// <summary>
+        /// Get the <see cref="BagEnumerator"/> for this <see cref="Bag{T}"/>. 
+        /// </summary>
+        /// <returns></returns>
+        /// <remarks>
+        /// Use this method preferentially over <see cref="IEnumerable.GetEnumerator"/> while enumerating via foreach
+        /// to avoid boxing the enumerator on every iteration, which can be expensive in high-performance environments.
+        /// </remarks>
+        public BagEnumerator GetEnumerator()
         {
             return new BagEnumerator(this);
         }
 
-        IEnumerator IEnumerable.GetEnumerator()
+        /// <summary>
+        /// Enumerates a Bag.
+        /// </summary>
+        public struct BagEnumerator : IEnumerator<T>
         {
-            return new BagEnumerator(this);
-        }
-
-        internal struct BagEnumerator : IEnumerator<T>
-        {
-            private volatile Bag<T> _bag;
+            private readonly Bag<T> _bag;
             private volatile int _index;
 
+            /// <summary>
+            /// Creates a new <see cref="BagEnumerator"/> for this <see cref="Bag{T}"/>.
+            /// </summary>
+            /// <param name="bag"></param>
             public BagEnumerator(Bag<T> bag)
             {
                 _bag = bag;
                 _index = -1;
             }
 
-            T IEnumerator<T>.Current => _bag[_index];
-            object IEnumerator.Current => _bag[_index];
+            readonly T IEnumerator<T>.Current => _bag[_index];
 
+            readonly object IEnumerator.Current => _bag[_index];
+
+            /// <summary>
+            /// Gets the element in the <see cref="Bag{T}"/> at the current position of the enumerator.
+            /// </summary>
+            public readonly T Current => _bag[_index];
+
+            /// <inheritdoc/>
             public bool MoveNext()
             {
                 return ++_index < _bag.Count;
             }
 
-            public void Dispose()
+            /// <inheritdoc/>
+            public readonly void Dispose()
             {
             }
 
-            public void Reset()
+            /// <inheritdoc/>
+            public readonly void Reset()
             {
             }
         }

src/cs/Tests/MonoGame.Extended.Tests/Collections/BagTests.cs

@@ -0,0 +1,42 @@
+using MonoGame.Extended.Collections;
+using System;
+using System.Collections.Generic;
+using System.Diagnostics;
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+using Xunit;
+
+namespace MonoGame.Extended.Tests.Collections
+{
+    public class BagTests
+    {
+        [Fact]
+        public void Bag_Enumeration_Does_Not_Allocate()
+        {
+            var bag = new Bag<int>();
+            for (int i = 0; i < 100; i++) bag.Add(i);
+            // ensure we have plenty of memory and that the heap only increases for the duration of this test
+            Assert.True(GC.TryStartNoGCRegion(Unsafe.SizeOf<Bag<int>.BagEnumerator>() * 1000));
+            var heapSize = GC.GetAllocatedBytesForCurrentThread();
+
+            // this should NOT allocate
+            foreach (int i in bag)
+            {
+                // assert methods cause the NoGCRegion to fail, so do this manually
+                if (GC.GetAllocatedBytesForCurrentThread() != heapSize)
+                    Assert.True(false);
+            }
+
+            // sanity check: this SHOULD allocate
+            foreach (int _ in (IEnumerable<int>)bag)
+            {
+                // assert methods cause the NoGCRegion to fail, so do this manually
+                if (GC.GetAllocatedBytesForCurrentThread() == heapSize)
+                    Assert.True(false);
+            }
+
+            GC.EndNoGCRegion();
+        }
+
+    }
+}