Bring back old array enumerator code #88371

MichalStrehovsky · 2023-07-04T07:16:26Z

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 MethodTable 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.

Cc @dotnet/ilc-contrib

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 dotnet#82751, but reintroduced the old classes. Fixes dotnet#82993.

ghost · 2023-07-04T07:16:39Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

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 MethodTable 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.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	`area-NativeAOT-coreclr`
Milestone:	-

jkotas · 2023-07-04T14:52:38Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs

+            }
+        }
+
+        void IEnumerator.Reset()


Should this be in the base class - same as in the shared one?

jkotas · 2023-07-04T15:00:40Z

looks like the new enumerators force array MethodTable for cases where we could have avoided them

What are those cases? The array enumerator should be only reachable if we have created the array.

jkotas · 2023-07-04T15:06:37Z

What would be the downside of using the native AOT enumerator everywhere? The only measurable downside that I can see is that the enumerator bbject would be 4 bytes larger on 32-bit platforms that we do not care about a whole lot.

MichalPetryka · 2023-07-04T18:38:14Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs

+            {
+                if ((uint)_index >= (uint)_endIndex)
+                    ThrowHelper.ThrowInvalidOperationException();
+                return _array[_index];


This will bounds check on every access, should this maybe use GetArrayDataReference and Unsafe.Add to avoid doing so?

Is this enumerator used often enough on hot paths for it to make a meaningful difference?

In general, we have stayed away from manual bounds-check elimination using unsafe code in collections.

Is this enumerator used often enough on hot paths for it to make a meaningful difference?

I'd say that arrays are fairly often passed to methods taking IEnumerable<T> and this would regress all such usages.

In general, we have stayed away from manual bounds-check elimination using unsafe code in collections.

Looks like we have an explicit bounds check on line 1244 now, so eliminating the one built into array accesses would be safe and worthwhile here.

IDisposable · 2023-07-05T17:16:16Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs

+        protected int _index;
+        protected int _endIndex;
+
+        internal SZGenericArrayEnumeratorBase()


would it be better to capture the _endIndex in the construction as the whole point of this base class is to inline and read-only the the length?

IDisposable · 2023-07-05T17:22:13Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Array.NativeAot.cs

+            }
+        }
+
+        object IEnumerator.Current


Is this style change now preferred to the other way in line 143?

object? IEnumerator.Current => Current;

MichalStrehovsky · 2023-07-05T21:36:59Z

looks like the new enumerators force array MethodTable for cases where we could have avoided them

What are those cases? The array enumerator should be only reachable if we have created the array.

Generic dictionaries end up referring to the MethodTable in the diff I was looking at. I didn't investigate how exactly, but it's plausible in the case where the scanner ended up scanning more stuff than ended up needed.

What would be the downside of using the native AOT enumerator everywhere? The only measurable downside that I can see is that the enumerator bbject would be 4 bytes larger on 32-bit platforms that we do not care about a whole lot.

If we don't care about that, I can unify this. I only wanted to restore the old code to avoid sidetracking this PR into any sort of runtime perf conversation - this is restoring code that Native AOT shipped with on .NET 7 so it doesn't do anything with runtime perf there. If we do this for all runtimes, this suddenly becomes a runtime perf conversation. The point of this PR is just to fix the size regression.

jkotas · 2023-07-05T22:05:15Z

Could you please try to do this for all runtimes without ifdef?

IDisposable

Nice

src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs

jkotas · 2023-07-06T06:39:27Z

src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs

            _index = -1;
+            _endIndex = endIndex;
        }

        public bool MoveNext()


public bool MoveNext() { int index = _index + 1; if ((uint)index < (uint)_endIndex) { _index = index; return true; } _index = _endIndex; return false; }

This should be better, on some architectures at least. (One less comparison.)

jkotas · 2023-07-06T06:43:50Z

I have run a quick microbenchmark

int Sum(IEnumerable<int> a)
{
    int sum = 0;
    foreach (var i in a) sum += i;
    return sum;
}

called on int[1000]

Surprisingly, this implementation is faster on this microbenchmark on my machine.

src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs

jkotas

Thanks

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

MichalStrehovsky · 2023-07-07T07:38:18Z

Surprisingly, this implementation is faster on this microbenchmark on my machine.

Nice, thanks for measuring! That's an unexpected bonus.

The failures are all #88499, merging.

MichalStrehovsky added the area-NativeAOT-coreclr label Jul 4, 2023

ghost assigned MichalStrehovsky Jul 4, 2023

jkotas reviewed Jul 4, 2023

View reviewed changes

MichalPetryka reviewed Jul 4, 2023

View reviewed changes

IDisposable reviewed Jul 5, 2023

View reviewed changes

FB

fcf50c8

MichalStrehovsky requested a review from marek-safar as a code owner July 6, 2023 00:34

build-analysis bot mentioned this pull request Jul 6, 2023

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed

IDisposable approved these changes Jul 6, 2023

View reviewed changes

jkotas reviewed Jul 6, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs Outdated Show resolved Hide resolved

jkotas reviewed Jul 6, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs Outdated Show resolved Hide resolved

jkotas approved these changes Jul 6, 2023

View reviewed changes

MichalStrehovsky and others added 2 commits July 7, 2023 12:31

Update Array.Enumerators.cs

b80ec8a

Apply suggestions from code review

9c60829

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

build-analysis bot mentioned this pull request Jul 7, 2023

simpleruntimeeventvalidation test failing in CI #88499

Closed

MichalStrehovsky merged commit 4285e43 into dotnet:main Jul 7, 2023

MichalStrehovsky deleted the fix82993 branch July 7, 2023 07:38

runfoapp bot mentioned this pull request Jul 7, 2023

Infra improvements for Helix #68176

Closed

ghost locked as resolved and limited conversation to collaborators Aug 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring back old array enumerator code #88371

Bring back old array enumerator code #88371

MichalStrehovsky commented Jul 4, 2023

ghost commented Jul 4, 2023

jkotas Jul 4, 2023 •

edited

Loading

jkotas commented Jul 4, 2023

jkotas commented Jul 4, 2023

MichalPetryka Jul 4, 2023

jkotas Jul 4, 2023

MichalPetryka Jul 4, 2023

IDisposable Jul 5, 2023

IDisposable Jul 5, 2023

IDisposable Jul 5, 2023

MichalStrehovsky commented Jul 5, 2023

jkotas commented Jul 5, 2023

IDisposable left a comment

jkotas Jul 6, 2023

jkotas commented Jul 6, 2023

jkotas left a comment

MichalStrehovsky commented Jul 7, 2023

Bring back old array enumerator code #88371

Bring back old array enumerator code #88371

Conversation

MichalStrehovsky commented Jul 4, 2023

ghost commented Jul 4, 2023

jkotas Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

jkotas commented Jul 4, 2023

jkotas commented Jul 4, 2023

MichalPetryka Jul 4, 2023

Choose a reason for hiding this comment

jkotas Jul 4, 2023

Choose a reason for hiding this comment

MichalPetryka Jul 4, 2023

Choose a reason for hiding this comment

IDisposable Jul 5, 2023

Choose a reason for hiding this comment

IDisposable Jul 5, 2023

Choose a reason for hiding this comment

IDisposable Jul 5, 2023

Choose a reason for hiding this comment

MichalStrehovsky commented Jul 5, 2023

jkotas commented Jul 5, 2023

IDisposable left a comment

Choose a reason for hiding this comment

jkotas Jul 6, 2023

Choose a reason for hiding this comment

jkotas commented Jul 6, 2023

jkotas left a comment

Choose a reason for hiding this comment

MichalStrehovsky commented Jul 7, 2023

jkotas Jul 4, 2023 •

edited

Loading