Reduce allocations around empty ReadOnlyCollections (#76097)

* Reduce allocations around empty ReadOnlyCollections

- Add an internal (for now) `ReadOnlyCollection<T>.Empty`
- Return the singleton `ReadOnlyCollection<T>.Empty` from `Array.AsReadOnly` when the source is empty
- Return a singleton empty enumerator from an empty `ReadOnlyCollection<T>`
- Fix up some reflection code to avoid unnecessary `ReadOnlyCollection<T>` allocations (e.g. just to wrap empty arrays)
- Fix a null reference bug in RuntimeCustomAttributeData.NamedArguments (I don't know if it's actually reachable, but this property shouldn't be returning null)
- Avoid a few unnecessary allocations in TimeZoneInfo.GetSystemTimeZones

* Address feedback, fix faulty tests, improve ConcurrentDictionary

* Fix reflection test expecting specific type for IList

* Address PR feedback

8 files changed, 86 insertions, 34 deletions

diff --git a/src/libraries/Common/tests/System/Collections/IDictionary.Generic.Tests.cs b/src/libraries/Common/tests/System/Collections/IDictionary.Generic.Tests.cs
index d701197d848..ce7682dd36f 100644
--- a/src/libraries/Common/tests/System/Collections/IDictionary.Generic.Tests.cs
+++ b/src/libraries/Common/tests/System/Collections/IDictionary.Generic.Tests.cs
@@ -411,10 +411,12 @@ namespace System.Collections.Tests
             }
             else
             {
-                keysEnum.MoveNext();
+                if (keysEnum.MoveNext())
+                {
+                    _ = keysEnum.Current;
+                }
                 keysEnum.Reset();
             }
-            var cur = keysEnum.Current;
         }
 
         [Theory]
@@ -506,10 +508,12 @@ namespace System.Collections.Tests
             }
             else
             {
-                valuesEnum.MoveNext();
+                if (valuesEnum.MoveNext())
+                {
+                    _ = valuesEnum.Current;
+                }
                 valuesEnum.Reset();
             }
-            var cur = valuesEnum.Current;
         }
 
         [Theory]
diff --git a/src/libraries/Common/tests/System/Collections/IEnumerable.Generic.Tests.cs b/src/libraries/Common/tests/System/Collections/IEnumerable.Generic.Tests.cs
index 624feb798a3..ba5810a7f1e 100644
--- a/src/libraries/Common/tests/System/Collections/IEnumerable.Generic.Tests.cs
+++ b/src/libraries/Common/tests/System/Collections/IEnumerable.Generic.Tests.cs
@@ -63,6 +63,18 @@ namespace System.Collections.Tests
         protected virtual bool Enumerator_Current_UndefinedOperation_Throws => false;
 
         /// <summary>
+        /// When calling Current of the empty enumerator before the first MoveNext, after the end of the collection,
+        /// or after modification of the enumeration, the resulting behavior is undefined. Tests are included
+        /// to cover two behavioral scenarios:
+        ///   - Throwing an InvalidOperationException
+        ///   - Returning an undefined value.
+        ///
+        /// If this property is set to true, the tests ensure that the exception is thrown. The default value is
+        /// <see cref="Enumerator_Current_UndefinedOperation_Throws"/>.
+        /// </summary>
+        protected virtual bool Enumerator_Empty_Current_UndefinedOperation_Throws => Enumerator_Current_UndefinedOperation_Throws;
+
+        /// <summary>
         /// When calling MoveNext or Reset after modification of the enumeration, the resulting behavior is
         /// undefined. Tests are included to cover two behavioral scenarios:
         ///   - Throwing an InvalidOperationException
@@ -590,7 +602,8 @@ namespace System.Collections.Tests
             IEnumerable<T> enumerable = GenericIEnumerableFactory(count);
             using (IEnumerator<T> enumerator = enumerable.GetEnumerator())
             {
-                if (Enumerator_Current_UndefinedOperation_Throws)
+                if (Enumerator_Current_UndefinedOperation_Throws ||
+                    (count == 0 && Enumerator_Empty_Current_UndefinedOperation_Throws))
                     Assert.Throws<InvalidOperationException>(() => enumerator.Current);
                 else
                     current = enumerator.Current;
@@ -606,7 +619,8 @@ namespace System.Collections.Tests
             using (IEnumerator<T> enumerator = enumerable.GetEnumerator())
             {
                 while (enumerator.MoveNext()) ;
-                if (Enumerator_Current_UndefinedOperation_Throws)
+                if (Enumerator_Current_UndefinedOperation_Throws ||
+                    (count == 0 && Enumerator_Empty_Current_UndefinedOperation_Throws))
                     Assert.Throws<InvalidOperationException>(() => enumerator.Current);
                 else
                     current = enumerator.Current;
diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs
index d69b5cf5e2a..b5edb1e448f 100644
--- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs
+++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs
@@ -2097,15 +2097,23 @@ namespace System.Collections.Concurrent
                     ThrowHelper.ThrowOutOfMemoryException();
                 }
 
-                var keys = new List<TKey>(count);
+                if (count == 0)
+                {
+                    // TODO https://github.com/dotnet/runtime/issues/76028: Replace with ReadOnlyCollection<TKey>.Empty.
+                    return Array.Empty<TKey>().AsReadOnly();
+                }
+
+                var keys = new TKey[count];
                 Node?[] buckets = _tables._buckets;
+                int n = 0;
                 for (int i = 0; i < buckets.Length; i++)
                 {
                     for (Node? current = buckets[i]; current != null; current = current._next)
                     {
-                        keys.Add(current._key);
+                        keys[n++] = current._key;
                     }
                 }
+                Debug.Assert(n == count);
 
                 return new ReadOnlyCollection<TKey>(keys);
             }
@@ -2131,15 +2139,23 @@ namespace System.Collections.Concurrent
                     ThrowHelper.ThrowOutOfMemoryException();
                 }
 
-                var values = new List<TValue>(count);
+                if (count == 0)
+                {
+                    // TODO https://github.com/dotnet/runtime/pull/76097:  Replace with ReadOnlyCollection<TValue>.Empty.
+                    return Array.Empty<TValue>().AsReadOnly();
+                }
+
+                var values = new TValue[count];
                 Node?[] buckets = _tables._buckets;
+                int n = 0;
                 for (int i = 0; i < buckets.Length; i++)
                 {
                     for (Node? current = buckets[i]; current != null; current = current._next)
                     {
-                        values.Add(current._value);
+                        values[n++] = current._value;
                     }
                 }
+                Debug.Assert(n == count);
 
                 return new ReadOnlyCollection<TValue>(values);
             }
diff --git a/src/libraries/System.Collections/tests/Generic/List/List.Generic.cs b/src/libraries/System.Collections/tests/Generic/List/List.Generic.cs
index 217052d0346..0a2f1fe239b 100644
--- a/src/libraries/System.Collections/tests/Generic/List/List.Generic.cs
+++ b/src/libraries/System.Collections/tests/Generic/List/List.Generic.cs
@@ -50,6 +50,8 @@ namespace System.Collections.Tests
         }
 
         protected override IEnumerable<ModifyEnumerable> GetModifyEnumerables(ModifyOperation operations) => new List<ModifyEnumerable>();
+        
+        protected override bool Enumerator_Empty_Current_UndefinedOperation_Throws => true;
     }
 
     public class List_Generic_Tests_int_ReadOnly : List_Generic_Tests<int>
@@ -73,5 +75,7 @@ namespace System.Collections.Tests
         }
 
         protected override IEnumerable<ModifyEnumerable> GetModifyEnumerables(ModifyOperation operations) => new List<ModifyEnumerable>();
+
+        protected override bool Enumerator_Empty_Current_UndefinedOperation_Throws => true;
     }
 }
diff --git a/src/libraries/System.Private.CoreLib/src/System/Array.cs b/src/libraries/System.Private.CoreLib/src/System/Array.cs
index b665d3094e1..eac471e826e 100644
--- a/src/libraries/System.Private.CoreLib/src/System/Array.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Array.cs
@@ -32,8 +32,9 @@ namespace System
                 ThrowHelper.ThrowArgumentNullException(ExceptionArgument.array);
             }
 
-            // T[] implements IList<T>.
-            return new ReadOnlyCollection<T>(array);
+            return array.Length == 0 ?
+                ReadOnlyCollection<T>.Empty :
+                new ReadOnlyCollection<T>(array);
         }
 
         public static void Resize<T>([NotNull] ref T[]? array, int newSize)
diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyCollection.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyCollection.cs
index 09f2bc62299..1107dfae26f 100644
--- a/src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyCollection.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyCollection.cs
@@ -23,6 +23,11 @@ namespace System.Collections.ObjectModel
             this.list = list;
         }
 
+        // TODO https://github.com/dotnet/runtime/issues/76028: Make this public.
+        /// <summary>Gets an empty <see cref="ReadOnlyCollection{T}"/>.</summary>
+        /// <remarks>The returned instance is immutable and will always be empty.</remarks>
+        internal static ReadOnlyCollection<T> Empty { get; } = new ReadOnlyCollection<T>(Array.Empty<T>());
+
         public int Count => list.Count;
 
         public T this[int index] => list[index];
@@ -37,10 +42,10 @@ namespace System.Collections.ObjectModel
             list.CopyTo(array, index);
         }
 
-        public IEnumerator<T> GetEnumerator()
-        {
-            return list.GetEnumerator();
-        }
+        public IEnumerator<T> GetEnumerator() =>
+            list.Count == 0 ?
+                SZGenericArrayEnumerator<T>.Empty :
+                list.GetEnumerator();
 
         public int IndexOf(T value)
         {
diff --git a/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs b/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
index 236442359f0..b3e8964bdb5 100644
--- a/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
@@ -775,29 +775,29 @@ namespace System
                     PopulateAllSystemTimeZones(cachedData);
                     cachedData._allSystemTimeZonesRead = true;
 
-                    List<TimeZoneInfo> list;
                     if (cachedData._systemTimeZones != null)
                     {
                         // return a collection of the cached system time zones
-                        list = new List<TimeZoneInfo>(cachedData._systemTimeZones.Values);
+                        TimeZoneInfo[] array = new TimeZoneInfo[cachedData._systemTimeZones.Count];
+                        cachedData._systemTimeZones.Values.CopyTo(array, 0);
+
+                        // sort and copy the TimeZoneInfo's into a ReadOnlyCollection for the user
+                        Array.Sort(array, static (x, y) =>
+                        {
+                            // sort by BaseUtcOffset first and by DisplayName second - this is similar to the Windows Date/Time control panel
+                            int comparison = x.BaseUtcOffset.CompareTo(y.BaseUtcOffset);
+                            return comparison == 0 ? string.CompareOrdinal(x.DisplayName, y.DisplayName) : comparison;
+                        });
+
+                        cachedData._readOnlySystemTimeZones = new ReadOnlyCollection<TimeZoneInfo>(array);
                     }
                     else
                     {
-                        // return an empty collection
-                        list = new List<TimeZoneInfo>();
+                        cachedData._readOnlySystemTimeZones = ReadOnlyCollection<TimeZoneInfo>.Empty;
                     }
-
-                    // sort and copy the TimeZoneInfo's into a ReadOnlyCollection for the user
-                    list.Sort(static (x, y) =>
-                    {
-                        // sort by BaseUtcOffset first and by DisplayName second - this is similar to the Windows Date/Time control panel
-                        int comparison = x.BaseUtcOffset.CompareTo(y.BaseUtcOffset);
-                        return comparison == 0 ? string.CompareOrdinal(x.DisplayName, y.DisplayName) : comparison;
-                    });
-
-                    cachedData._readOnlySystemTimeZones = new ReadOnlyCollection<TimeZoneInfo>(list);
                 }
             }
+
             return cachedData._readOnlySystemTimeZones;
         }
 
diff --git a/src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/CustomAttributes/CustomAttributeTests.cs b/src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/CustomAttributes/CustomAttributeTests.cs
index 15d1fa8d2fc..c8661c8b1ba 100644
--- a/src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/CustomAttributes/CustomAttributeTests.cs
+++ b/src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/CustomAttributes/CustomAttributeTests.cs
@@ -146,8 +146,12 @@ namespace System.Reflection.Tests
                 {
                     Assert.NotSame(cad1.ConstructorArguments, cad2.ConstructorArguments);
                 }
-                Assert.True(cad1.ConstructorArguments is ReadOnlyCollection<CustomAttributeTypedArgument>);
-                Assert.True(cad2.ConstructorArguments is ReadOnlyCollection<CustomAttributeTypedArgument>);
+                Assert.True(
+                    (cad1.ConstructorArguments is ReadOnlyCollection<CustomAttributeTypedArgument>) ||
+                    (cad1.ConstructorArguments is CustomAttributeTypedArgument[] cataArr1 && cataArr1.Length == 0));
+                Assert.True(
+                    (cad2.ConstructorArguments is ReadOnlyCollection<CustomAttributeTypedArgument>) ||
+                    (cad2.ConstructorArguments is CustomAttributeTypedArgument[] cataArr2 && cataArr2.Length == 0));
                 for (int j = 0; j < cad1.ConstructorArguments.Count; j++)
                 {
                     cad1.ConstructorArguments[j].ValidateEqualButFreshlyAllocated(cad2.ConstructorArguments[j]);
@@ -158,8 +162,12 @@ namespace System.Reflection.Tests
                 {
                     Assert.NotSame(cad1.NamedArguments, cad2.NamedArguments);
                 }
-                Assert.True(cad1.NamedArguments is ReadOnlyCollection<CustomAttributeNamedArgument>);
-                Assert.True(cad2.NamedArguments is ReadOnlyCollection<CustomAttributeNamedArgument>);
+                Assert.True(
+                    (cad1.NamedArguments is ReadOnlyCollection<CustomAttributeNamedArgument>) ||
+                    (cad1.NamedArguments is CustomAttributeNamedArgument[] canaArr1 && canaArr1.Length == 0));
+                Assert.True(
+                    (cad2.NamedArguments is ReadOnlyCollection<CustomAttributeNamedArgument>) ||
+                    (cad2.NamedArguments is CustomAttributeNamedArgument[] canaArr2 && canaArr2.Length == 0));
                 for (int j = 0; j < cad1.NamedArguments.Count; j++)
                 {
                     cad1.NamedArguments[j].TypedValue.ValidateEqualButFreshlyAllocated(cad2.NamedArguments[j].TypedValue);