From 0d316aefa10dc69e706353405906ef126ce9e20e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 4 Jul 2023 16:15:34 +0900 Subject: [PATCH 1/4] Bring back old array enumerator code This is a 0.3% size saving for Stage1. We not only allow array enumerators to be preinitialized again, but also avoid introducing many array `MethodTables` (looks like the new enumerators force array MethodTables for cases where we could have avoided them). I shaped the old code to look like after the refactor in #82751, but reintroduced the old classes. Fixes #82993. --- .../src/System/Array.NativeAot.cs | 67 ++++++++++++++++++- .../src/System/Array.Enumerators.cs | 3 + 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs index f2d1aac03130bf..87ea8efaae58b0 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs @@ -1099,7 +1099,10 @@ private Array() { } public new IEnumerator GetEnumerator() { T[] @this = Unsafe.As(this); - return @this.Length == 0 ? SZGenericArrayEnumerator.Empty : new SZGenericArrayEnumerator(@this); + // get length so we don't have to call the Length property again in ArrayEnumerator constructor + // and avoid more checking there too. + int length = @this.Length; + return length == 0 ? SZGenericArrayEnumerator.Empty : new SZGenericArrayEnumerator(@this, length); } public int Count @@ -1196,6 +1199,68 @@ public void RemoveAt(int index) } } + internal class SZGenericArrayEnumeratorBase : IDisposable + { + protected int _index; + protected int _endIndex; + + internal SZGenericArrayEnumeratorBase() + { + _index = -1; + } + + public bool MoveNext() + { + if (_index < _endIndex) + { + _index++; + return (_index < _endIndex); + } + return false; + } + + public void Dispose() + { + } + } + + internal sealed class SZGenericArrayEnumerator : SZGenericArrayEnumeratorBase, IEnumerator + { + private readonly T[] _array; + + // Passing -1 for endIndex so that MoveNext always returns false without mutating _index + internal static readonly SZGenericArrayEnumerator Empty = new SZGenericArrayEnumerator(null, -1); + + internal SZGenericArrayEnumerator(T[] array, int endIndex) + { + _array = array; + _endIndex = endIndex; + } + + public T Current + { + get + { + if ((uint)_index >= (uint)_endIndex) + ThrowHelper.ThrowInvalidOperationException(); + return _array[_index]; + } + } + + object IEnumerator.Current + { + get + { + return Current; + } + } + + void IEnumerator.Reset() + { + _index = -1; + } + } + public class MDArray { public const int MinRank = 1; diff --git a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs index 90542e9685cf5c..99f32ea0d1c03a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs @@ -67,6 +67,8 @@ public void Reset() } } + // Native AOT doesn't use these for size on disk reasons +#if !NATIVEAOT internal abstract class SZGenericArrayEnumeratorBase : IDisposable { protected readonly Array _array; @@ -140,6 +142,7 @@ public T Current object? IEnumerator.Current => Current; } +#endif internal abstract class GenericEmptyEnumeratorBase : IDisposable, IEnumerator { From fcf50c85ffdde76f6befebe7b3b1e086f6e39a09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 6 Jul 2023 09:34:43 +0900 Subject: [PATCH 2/4] FB --- .../src/System/Array.CoreCLR.cs | 3 +- .../src/System/Array.NativeAot.cs | 62 ------------------- .../src/System/Array.Enumerators.cs | 53 ++++++---------- .../src/System/Array.Mono.cs | 3 +- 4 files changed, 22 insertions(+), 99 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs index 8a32e8f646d1a5..699f99c441fdf4 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs @@ -400,7 +400,8 @@ internal IEnumerator GetEnumerator() // ! Warning: "this" is an array, not an SZArrayHelper. See comments above // ! or you may introduce a security hole! T[] @this = Unsafe.As(this); - return @this.Length == 0 ? SZGenericArrayEnumerator.Empty : new SZGenericArrayEnumerator(@this); + int length = @this.Length; + return length == 0 ? SZGenericArrayEnumerator.Empty : new SZGenericArrayEnumerator(@this, length); } private void CopyTo(T[] array, int index) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs index 87ea8efaae58b0..1cb5604cefbd58 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs @@ -1199,68 +1199,6 @@ public void RemoveAt(int index) } } - internal class SZGenericArrayEnumeratorBase : IDisposable - { - protected int _index; - protected int _endIndex; - - internal SZGenericArrayEnumeratorBase() - { - _index = -1; - } - - public bool MoveNext() - { - if (_index < _endIndex) - { - _index++; - return (_index < _endIndex); - } - return false; - } - - public void Dispose() - { - } - } - - internal sealed class SZGenericArrayEnumerator : SZGenericArrayEnumeratorBase, IEnumerator - { - private readonly T[] _array; - - // Passing -1 for endIndex so that MoveNext always returns false without mutating _index - internal static readonly SZGenericArrayEnumerator Empty = new SZGenericArrayEnumerator(null, -1); - - internal SZGenericArrayEnumerator(T[] array, int endIndex) - { - _array = array; - _endIndex = endIndex; - } - - public T Current - { - get - { - if ((uint)_index >= (uint)_endIndex) - ThrowHelper.ThrowInvalidOperationException(); - return _array[_index]; - } - } - - object IEnumerator.Current - { - get - { - return Current; - } - } - - void IEnumerator.Reset() - { - _index = -1; - } - } - public class MDArray { public const int MinRank = 1; diff --git a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs index 99f32ea0d1c03a..5ffe1d5bf04e5f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs @@ -67,82 +67,65 @@ public void Reset() } } - // Native AOT doesn't use these for size on disk reasons -#if !NATIVEAOT internal abstract class SZGenericArrayEnumeratorBase : IDisposable { - protected readonly Array _array; protected int _index; + protected readonly int _endIndex; - protected SZGenericArrayEnumeratorBase(Array array) + protected SZGenericArrayEnumeratorBase(int endIndex) { - Debug.Assert(array != null); - - _array = array; _index = -1; + _endIndex = endIndex; } public bool MoveNext() { - int index = _index + 1; - uint length = (uint)_array.NativeLength; - if ((uint)index >= length) + if (_index < _endIndex) { - _index = (int)length; - return false; + _index++; + return (_index < _endIndex); } - _index = index; - return true; + return false; } public void Reset() => _index = -1; -#pragma warning disable CA1822 // https://github.com/dotnet/roslyn-analyzers/issues/5911 public void Dispose() { } -#pragma warning restore CA1822 } internal sealed class SZGenericArrayEnumerator : SZGenericArrayEnumeratorBase, IEnumerator { + private readonly T[]? _array; + /// Provides an empty enumerator singleton. /// /// If the consumer is using SZGenericArrayEnumerator elsewhere or is otherwise likely /// to be using T[] elsewhere, this singleton should be used. Otherwise, GenericEmptyEnumerator's /// singleton should be used instead, as it doesn't reference T[] in order to reduce footprint. /// -#pragma warning disable CA1825 - internal static readonly SZGenericArrayEnumerator Empty = - // Array.Empty is intentionally omitted here, since we don't want to pay for generic instantiations - // that wouldn't have otherwise been used. - new SZGenericArrayEnumerator(new T[0]); -#pragma warning restore CA1825 - - public SZGenericArrayEnumerator(T[] array) - : base(array) + internal static readonly SZGenericArrayEnumerator Empty = new SZGenericArrayEnumerator(null, -1); + + internal SZGenericArrayEnumerator(T[]? array, int endIndex) + : base(endIndex) { + Debug.Assert(array == null || endIndex == array.Length); + _array = array; } public T Current { get { - int index = _index; - T[] array = Unsafe.As(_array); - - if ((uint)index >= (uint)array.Length) - { - ThrowHelper.ThrowInvalidOperationException_EnumCurrent(index); - } - - return array[index]; + if ((uint)_index >= (uint)_endIndex) + ThrowHelper.ThrowInvalidOperationException(); + return _array![_index]; } } object? IEnumerator.Current => Current; } -#endif internal abstract class GenericEmptyEnumeratorBase : IDisposable, IEnumerator { diff --git a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs index af3e7f05fc1ab8..f6782a42a23be3 100644 --- a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs @@ -468,7 +468,8 @@ internal bool InternalArray__ICollection_get_IsReadOnly() internal IEnumerator InternalArray__IEnumerable_GetEnumerator() { - return Length == 0 ? SZGenericArrayEnumerator.Empty : new SZGenericArrayEnumerator(Unsafe.As(this)); + int length = Length; + return length == 0 ? SZGenericArrayEnumerator.Empty : new SZGenericArrayEnumerator(Unsafe.As(this), length); } internal void InternalArray__ICollection_Clear() From b80ec8a0d07ac914d81875036447a1713c0d5b20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 7 Jul 2023 12:31:13 +0900 Subject: [PATCH 3/4] Update Array.Enumerators.cs --- .../src/System/Array.Enumerators.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs index 5ffe1d5bf04e5f..6946dfe9201559 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs @@ -80,11 +80,13 @@ protected SZGenericArrayEnumeratorBase(int endIndex) public bool MoveNext() { - if (_index < _endIndex) + int index = _index + 1; + if ((uint)index < (uint)_endIndex) { - _index++; - return (_index < _endIndex); + _index = index; + return true; } + _index = _endIndex; return false; } From 9c60829846ceea6c54f742c9704c7dd8fdb9dadf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 7 Jul 2023 12:31:41 +0900 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Jan Kotas --- .../System.Private.CoreLib/src/System/Array.Enumerators.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs index 6946dfe9201559..60aa83909d4b6a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs @@ -107,7 +107,7 @@ internal sealed class SZGenericArrayEnumerator : SZGenericArrayEnumeratorBase /// to be using T[] elsewhere, this singleton should be used. Otherwise, GenericEmptyEnumerator's /// singleton should be used instead, as it doesn't reference T[] in order to reduce footprint. /// - internal static readonly SZGenericArrayEnumerator Empty = new SZGenericArrayEnumerator(null, -1); + internal static readonly SZGenericArrayEnumerator Empty = new SZGenericArrayEnumerator(null, 0); internal SZGenericArrayEnumerator(T[]? array, int endIndex) : base(endIndex) @@ -121,7 +121,7 @@ public T Current get { if ((uint)_index >= (uint)_endIndex) - ThrowHelper.ThrowInvalidOperationException(); + ThrowHelper.ThrowInvalidOperationException_EnumCurrent(_index); return _array![_index]; } }