From d93d2e8ba37b8bd4f276e51abd054ebabf0234cd Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 27 Feb 2023 18:26:08 -0800 Subject: [PATCH 1/7] Unify Array enumerators between CoreCLR and NativeAOT Contributes to #82732 --- .../src/System/Array.NativeAot.cs | 88 ++----------------- .../src/System/Array.Enumerators.cs | 51 +++++++---- 2 files changed, 41 insertions(+), 98 deletions(-) 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 7798afdff80b32..4e106749c33875 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 @@ -1157,38 +1157,6 @@ private static int LastIndexOfImpl(T[] array, T value, int startIndex, int co } } - internal class ArrayEnumeratorBase : ICloneable - { - protected int _index; - protected int _endIndex; - - internal ArrayEnumeratorBase() - { - _index = -1; - } - - public bool MoveNext() - { - if (_index < _endIndex) - { - _index++; - return (_index < _endIndex); - } - return false; - } - - public object Clone() - { - return MemberwiseClone(); - } - -#pragma warning disable CA1822 // https://github.com/dotnet/roslyn-analyzers/issues/5911 - public void Dispose() - { - } -#pragma warning restore CA1822 - } - // // Note: the declared base type and interface list also determines what Reflection returns from TypeInfo.BaseType and TypeInfo.ImplementedInterfaces for array types. // This also means the class must be declared "public" so that the framework can reflect on it. @@ -1200,17 +1168,15 @@ private Array() { } public new IEnumerator GetEnumerator() { - // 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 ? ArrayEnumerator.Empty : new ArrayEnumerator(Unsafe.As(this), length); + T[] _this = Unsafe.As(this); + return _this.Length == 0 ? SZGenericArrayEnumerator.Empty : new SZGenericArrayEnumerator(_this); } public int Count { get { - return this.Length; + return Unsafe.As(this).Length; } } @@ -1240,13 +1206,14 @@ public void Clear() public bool Contains(T item) { - T[] array = Unsafe.As(this); - return Array.IndexOf(array, item, 0, array.Length) >= 0; + T[] _this = Unsafe.As(this); + return Array.IndexOf(_this, item, 0, _this.Length) >= 0; } public void CopyTo(T[] array, int arrayIndex) { - Array.Copy(Unsafe.As(this), 0, array, arrayIndex, this.Length); + T[] _this = Unsafe.As(this); + Array.Copy(_this, 0, array, arrayIndex, _this.Length); } public bool Remove(T item) @@ -1284,8 +1251,8 @@ public T this[int index] public int IndexOf(T item) { - T[] array = Unsafe.As(this); - return Array.IndexOf(array, item, 0, array.Length); + T[] _this = Unsafe.As(this); + return Array.IndexOf(_this, item, 0, _this.Length); } public void Insert(int index, T item) @@ -1297,43 +1264,6 @@ public void RemoveAt(int index) { ThrowHelper.ThrowNotSupportedException(); } - - private sealed class ArrayEnumerator : ArrayEnumeratorBase, IEnumerator - { - private readonly T[] _array; - - // Passing -1 for endIndex so that MoveNext always returns false without mutating _index - internal static readonly ArrayEnumerator Empty = new ArrayEnumerator(null, -1); - - internal ArrayEnumerator(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 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 04c972c0a3c45b..8431e98bef527a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs @@ -1,8 +1,10 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using System.Collections; using System.Collections.Generic; +using System.Runtime.CompilerServices; using System.Diagnostics; namespace System @@ -64,18 +66,12 @@ public void Reset() } } - internal sealed class SZGenericArrayEnumerator : IEnumerator + internal class SZGenericArrayEnumeratorBase { - private readonly T[] _array; - private int _index; + protected readonly Array _array; + protected int _index; - // Array.Empty is intentionally omitted here, since we don't want to pay for generic instantiations that - // wouldn't have otherwise been used. -#pragma warning disable CA1825 - internal static readonly SZGenericArrayEnumerator Empty = new SZGenericArrayEnumerator(new T[0]); -#pragma warning restore CA1825 - - internal SZGenericArrayEnumerator(T[] array) + protected SZGenericArrayEnumeratorBase(Array array) { Debug.Assert(array != null); @@ -86,21 +82,44 @@ internal SZGenericArrayEnumerator(T[] array) public bool MoveNext() { int index = _index + 1; - if ((uint)index >= (uint)_array.Length) + int length = (int)_array.NativeLength; + if ((uint)index >= (uint)length) { - _index = _array.Length; + _index = length; return false; } _index = index; return true; } + public void Reset() => _index = -1; + +#pragma warning disable CA1822 // Mark members as static + public void Dispose() + { + } +#pragma warning restore CA1822 + } + + internal sealed class SZGenericArrayEnumerator : SZGenericArrayEnumeratorBase, IEnumerator + { + // Array.Empty is intentionally omitted here, since we don't want to pay for generic instantiations that + // wouldn't have otherwise been used. +#pragma warning disable CA1825 + internal static readonly SZGenericArrayEnumerator Empty = new SZGenericArrayEnumerator(new T[0]); +#pragma warning restore CA1825 + + public SZGenericArrayEnumerator(T[] array) + : base(array) + { + } + public T Current { get { int index = _index; - T[] array = _array; + T[] array = Unsafe.As(_array); if ((uint)index >= (uint)array.Length) { @@ -112,11 +131,5 @@ public T Current } object? IEnumerator.Current => Current; - - void IEnumerator.Reset() => _index = -1; - - public void Dispose() - { - } } } From 186db65fe6d3f7a7ac0b2df8611253b582b81d38 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 1 Mar 2023 06:50:55 -0800 Subject: [PATCH 2/7] Update AOT compiler --- .../Compiler/DependencyAnalysis/NodeFactory.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs index eb5f77754b0413..aa5629d106503f 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs @@ -1057,7 +1057,7 @@ public TypeDesc ArrayOfTEnumeratorType { get { - _systemArrayOfTEnumeratorType ??= _systemArrayOfTEnumeratorType = ArrayOfTClass.GetNestedType("ArrayEnumerator"); + _systemArrayOfTEnumeratorType ??= _systemArrayOfTEnumeratorType = _context.SystemModule.GetKnownType("System", "SZGenericArrayEnumerator`1"); return _systemArrayOfTEnumeratorType; } } From 9d5829563329938a954ff94f98376ed66605aa01 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 1 Mar 2023 09:51:47 -0800 Subject: [PATCH 3/7] Microbenchmark tweaks --- .../src/System/Array.Enumerators.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 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 8431e98bef527a..748a6fdae7d1f4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs @@ -28,9 +28,10 @@ public object Clone() public bool MoveNext() { nint index = _index + 1; - if ((nuint)index >= _array.NativeLength) + nuint length = _array.NativeLength; + if ((nuint)index >= length) { - _index = (nint)_array.NativeLength; + _index = (nint)length; return false; } _index = index; @@ -82,10 +83,10 @@ protected SZGenericArrayEnumeratorBase(Array array) public bool MoveNext() { int index = _index + 1; - int length = (int)_array.NativeLength; - if ((uint)index >= (uint)length) + uint length = (uint)_array.NativeLength; + if ((uint)index >= length) { - _index = length; + _index = (int)length; return false; } _index = index; From 0880a1df595e8ca06be9ffc74e3a692dde48cd14 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 1 Mar 2023 10:00:24 -0800 Subject: [PATCH 4/7] Naming convention --- .../src/System/Array.NativeAot.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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 4e106749c33875..c587b0b6527efb 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 @@ -1168,8 +1168,8 @@ private Array() { } public new IEnumerator GetEnumerator() { - T[] _this = Unsafe.As(this); - return _this.Length == 0 ? SZGenericArrayEnumerator.Empty : new SZGenericArrayEnumerator(_this); + T[] @this = Unsafe.As(this); + return @this.Length == 0 ? SZGenericArrayEnumerator.Empty : new SZGenericArrayEnumerator(@this); } public int Count @@ -1206,14 +1206,14 @@ public void Clear() public bool Contains(T item) { - T[] _this = Unsafe.As(this); - return Array.IndexOf(_this, item, 0, _this.Length) >= 0; + T[] @this = Unsafe.As(this); + return Array.IndexOf(@this, item, 0, @this.Length) >= 0; } public void CopyTo(T[] array, int arrayIndex) { - T[] _this = Unsafe.As(this); - Array.Copy(_this, 0, array, arrayIndex, _this.Length); + T[] @this = Unsafe.As(this); + Array.Copy(@this, 0, array, arrayIndex, @this.Length); } public bool Remove(T item) @@ -1251,8 +1251,8 @@ public T this[int index] public int IndexOf(T item) { - T[] _this = Unsafe.As(this); - return Array.IndexOf(_this, item, 0, _this.Length); + T[] @this = Unsafe.As(this); + return Array.IndexOf(@this, item, 0, @this.Length); } public void Insert(int index, T item) From 80736fc5b1c6a611a8497a0d93d598c851ccf616 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 1 Mar 2023 10:03:35 -0800 Subject: [PATCH 5/7] Update src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs Co-authored-by: Stephen Toub --- .../Compiler/DependencyAnalysis/NodeFactory.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs index aa5629d106503f..36586d063dd5f9 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs @@ -1057,7 +1057,7 @@ public TypeDesc ArrayOfTEnumeratorType { get { - _systemArrayOfTEnumeratorType ??= _systemArrayOfTEnumeratorType = _context.SystemModule.GetKnownType("System", "SZGenericArrayEnumerator`1"); + _systemArrayOfTEnumeratorType ??= _context.SystemModule.GetKnownType("System", "SZGenericArrayEnumerator`1"); return _systemArrayOfTEnumeratorType; } } From 4af7314e512daa36647cb5ca6954cf635e4e7bf3 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 1 Mar 2023 10:20:27 -0800 Subject: [PATCH 6/7] Feedback, style --- .../src/System/Array.CoreCLR.cs | 32 +++++++++---------- .../DependencyAnalysis/NodeFactory.cs | 10 ++---- .../src/System/Array.Enumerators.cs | 2 +- 3 files changed, 20 insertions(+), 24 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 a473169d0dccdc..40dfe0c744c107 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs @@ -464,8 +464,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); + T[] @this = Unsafe.As(this); + return @this.Length == 0 ? SZGenericArrayEnumerator.Empty : new SZGenericArrayEnumerator(@this); } private void CopyTo(T[] array, int index) @@ -473,42 +473,42 @@ private void CopyTo(T[] array, int index) // ! Warning: "this" is an array, not an SZArrayHelper. See comments above // ! or you may introduce a security hole! - T[] _this = Unsafe.As(this); - Array.Copy(_this, 0, array, index, _this.Length); + T[] @this = Unsafe.As(this); + Array.Copy(@this, 0, array, index, @this.Length); } internal int get_Count() { // ! 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; + T[] @this = Unsafe.As(this); + return @this.Length; } internal T get_Item(int index) { // ! Warning: "this" is an array, not an SZArrayHelper. See comments above // ! or you may introduce a security hole! - T[] _this = Unsafe.As(this); - if ((uint)index >= (uint)_this.Length) + T[] @this = Unsafe.As(this); + if ((uint)index >= (uint)@this.Length) { ThrowHelper.ThrowArgumentOutOfRange_IndexMustBeLessException(); } - return _this[index]; + return @this[index]; } internal void set_Item(int index, T value) { // ! Warning: "this" is an array, not an SZArrayHelper. See comments above // ! or you may introduce a security hole! - T[] _this = Unsafe.As(this); - if ((uint)index >= (uint)_this.Length) + T[] @this = Unsafe.As(this); + if ((uint)index >= (uint)@this.Length) { ThrowHelper.ThrowArgumentOutOfRange_IndexMustBeLessException(); } - _this[index] = value; + @this[index] = value; } private void Add(T _) @@ -521,8 +521,8 @@ private bool Contains(T value) { // ! Warning: "this" is an array, not an SZArrayHelper. See comments above // ! or you may introduce a security hole! - T[] _this = Unsafe.As(this); - return Array.IndexOf(_this, value, 0, _this.Length) >= 0; + T[] @this = Unsafe.As(this); + return Array.IndexOf(@this, value, 0, @this.Length) >= 0; } private bool get_IsReadOnly() @@ -542,8 +542,8 @@ private int IndexOf(T value) { // ! Warning: "this" is an array, not an SZArrayHelper. See comments above // ! or you may introduce a security hole! - T[] _this = Unsafe.As(this); - return Array.IndexOf(_this, value, 0, _this.Length); + T[] @this = Unsafe.As(this); + return Array.IndexOf(@this, value, 0, @this.Length); } private void Insert(int _, T _1) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs index 36586d063dd5f9..7af72a0dcb2ab8 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs @@ -1047,8 +1047,7 @@ public MetadataType ArrayOfTClass { get { - _systemArrayOfTClass ??= _systemArrayOfTClass = _context.SystemModule.GetKnownType("System", "Array`1"); - return _systemArrayOfTClass; + return _systemArrayOfTClass ??= _context.SystemModule.GetKnownType("System", "Array`1"); } } @@ -1057,8 +1056,7 @@ public TypeDesc ArrayOfTEnumeratorType { get { - _systemArrayOfTEnumeratorType ??= _context.SystemModule.GetKnownType("System", "SZGenericArrayEnumerator`1"); - return _systemArrayOfTEnumeratorType; + return _systemArrayOfTEnumeratorType ??= _context.SystemModule.GetKnownType("System", "SZGenericArrayEnumerator`1"); } } @@ -1069,9 +1067,7 @@ public MethodDesc InstanceMethodRemovedHelper { // This helper is optional, but it's fine for this cache to be ineffective if that happens. // Those scenarios are rare and typically deal with small compilations. - _instanceMethodRemovedHelper ??= TypeSystemContext.GetOptionalHelperEntryPoint("ThrowHelpers", "ThrowInstanceBodyRemoved"); - - return _instanceMethodRemovedHelper; + return _instanceMethodRemovedHelper ??= TypeSystemContext.GetOptionalHelperEntryPoint("ThrowHelpers", "ThrowInstanceBodyRemoved"); } } 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 748a6fdae7d1f4..ddb472aaa8f1e4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs @@ -95,7 +95,7 @@ public bool MoveNext() public void Reset() => _index = -1; -#pragma warning disable CA1822 // Mark members as static +#pragma warning disable CA1822 // https://github.com/dotnet/roslyn-analyzers/issues/5911 public void Dispose() { } From 2e07b1457ff7f0857a453ca3dd3a7502f1729d03 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 1 Mar 2023 12:15:16 -0800 Subject: [PATCH 7/7] Fix tests --- .../Compiler/DependencyAnalysis/NodeFactory.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs index 7af72a0dcb2ab8..487f25faf0d3f0 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs @@ -1056,7 +1056,9 @@ public TypeDesc ArrayOfTEnumeratorType { get { - return _systemArrayOfTEnumeratorType ??= _context.SystemModule.GetKnownType("System", "SZGenericArrayEnumerator`1"); + // This type is optional, but it's fine for this cache to be ineffective if that happens. + // Those scenarios are rare and typically deal with small compilations. + return _systemArrayOfTEnumeratorType ??= _context.SystemModule.GetType("System", "SZGenericArrayEnumerator`1", throwIfNotFound: false); } }