From 903926f8274a57f38f77316ee534febda354b45e Mon Sep 17 00:00:00 2001 From: Keegan Caruso Date: Wed, 16 Oct 2024 14:59:51 -0700 Subject: [PATCH 1/6] Don't use a GVM in Linq Select with NAOT by default Adds a feature flag to allow Linq Select to not use a GVM implementation. --- .../System.Linq/src/System.Linq.csproj | 1 + .../src/System/Linq/IteratorOptions.cs | 16 +++++++++++++ .../src/System/Linq/OfType.SpeedOpt.cs | 2 +- .../System.Linq/src/System/Linq/Select.cs | 23 ++++++++++++++++++- .../build/Microsoft.NET.ILLink.targets | 1 + 5 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 src/libraries/System.Linq/src/System/Linq/IteratorOptions.cs diff --git a/src/libraries/System.Linq/src/System.Linq.csproj b/src/libraries/System.Linq/src/System.Linq.csproj index 79c10444f489b..32500dfa5ebd6 100644 --- a/src/libraries/System.Linq/src/System.Linq.csproj +++ b/src/libraries/System.Linq/src/System.Linq.csproj @@ -64,6 +64,7 @@ + diff --git a/src/libraries/System.Linq/src/System/Linq/IteratorOptions.cs b/src/libraries/System.Linq/src/System/Linq/IteratorOptions.cs new file mode 100644 index 0000000000000..226b84338337a --- /dev/null +++ b/src/libraries/System.Linq/src/System/Linq/IteratorOptions.cs @@ -0,0 +1,16 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics.CodeAnalysis; + +namespace System.Linq +{ + /// + /// Defines options for how iterators behave. + /// + internal static class IteratorOptions + { + [FeatureSwitchDefinition("System.Linq.ValueTypeTrimFriendlySelect")] + public static bool ValueTypeTrimFriendlySelect { get; } = AppContext.TryGetSwitch("System.Linq.ValueTypeTrimFriendlySelect", out bool isEnabled) ? isEnabled : true; + } +} diff --git a/src/libraries/System.Linq/src/System/Linq/OfType.SpeedOpt.cs b/src/libraries/System.Linq/src/System/Linq/OfType.SpeedOpt.cs index 73983cf87fbde..bbe3810251830 100644 --- a/src/libraries/System.Linq/src/System/Linq/OfType.SpeedOpt.cs +++ b/src/libraries/System.Linq/src/System/Linq/OfType.SpeedOpt.cs @@ -172,7 +172,7 @@ public override IEnumerable Select(Func s new IEnumerableWhereSelectIterator(objectSource, isTResult, localSelector); } - return base.Select(selector); + return SelectImplementation(selector, this); } } } diff --git a/src/libraries/System.Linq/src/System/Linq/Select.cs b/src/libraries/System.Linq/src/System/Linq/Select.cs index 37a41c450f873..e0f8f586f6cd8 100644 --- a/src/libraries/System.Linq/src/System/Linq/Select.cs +++ b/src/libraries/System.Linq/src/System/Linq/Select.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; +using System.Runtime.CompilerServices; using static System.Linq.Utilities; namespace System.Linq @@ -25,7 +26,7 @@ public static IEnumerable Select( if (source is Iterator iterator) { - return iterator.Select(selector); + return SelectImplementation(selector, iterator); } if (source is IList ilist) @@ -51,6 +52,26 @@ public static IEnumerable Select( return new IEnumerableSelectIterator(source, selector); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static IEnumerable SelectImplementation(Func selector, Iterator iterator) + { + // In Native AOT, GVMs with Value types are not trim friendly. + // If the option is enabled, we don't call the GVM for value types. We don't do the + // same for reference types because reference types are trim friendly with GVMs + // and it is preferable to call the GVM as it allows the select implementation to be + // specialized. + if (IteratorOptions.ValueTypeTrimFriendlySelect && typeof(TResult).IsValueType) + { +#if OPTIMIZE_FOR_SIZE + return new IEnumerableSelectIterator(iterator, selector); +#else + return new IteratorSelectIterator(iterator, selector); +#endif + } + + return iterator.Select(selector); + } + public static IEnumerable Select(this IEnumerable source, Func selector) { if (source is null) diff --git a/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets b/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets index 3fdf083a3beda..eff4d7bef6f30 100644 --- a/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets +++ b/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets @@ -38,6 +38,7 @@ Copyright (c) .NET Foundation. All rights reserved. false false false + true false false From 0827a708b2da60ee7cb2244e2afa9490780abf58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 19 Nov 2024 23:07:03 -0800 Subject: [PATCH 2/6] Temporary change so we can run testing on this --- src/libraries/System.Linq/src/System/Linq/Select.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Linq/src/System/Linq/Select.cs b/src/libraries/System.Linq/src/System/Linq/Select.cs index e0f8f586f6cd8..07cd6736645da 100644 --- a/src/libraries/System.Linq/src/System/Linq/Select.cs +++ b/src/libraries/System.Linq/src/System/Linq/Select.cs @@ -60,7 +60,7 @@ private static IEnumerable SelectImplementation(Func< // same for reference types because reference types are trim friendly with GVMs // and it is preferable to call the GVM as it allows the select implementation to be // specialized. - if (IteratorOptions.ValueTypeTrimFriendlySelect && typeof(TResult).IsValueType) + if (/*IteratorOptions.ValueTypeTrimFriendlySelect && */typeof(TResult).IsValueType) { #if OPTIMIZE_FOR_SIZE return new IEnumerableSelectIterator(iterator, selector); From 616d23c85c408eb953524df99f41842ab3df47cc Mon Sep 17 00:00:00 2001 From: Keegan Date: Tue, 26 Nov 2024 15:47:30 -0800 Subject: [PATCH 3/6] Update src/libraries/System.Linq/src/System/Linq/Select.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Michal Strehovský --- src/libraries/System.Linq/src/System/Linq/Select.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Linq/src/System/Linq/Select.cs b/src/libraries/System.Linq/src/System/Linq/Select.cs index 07cd6736645da..fac0978314cb6 100644 --- a/src/libraries/System.Linq/src/System/Linq/Select.cs +++ b/src/libraries/System.Linq/src/System/Linq/Select.cs @@ -55,11 +55,11 @@ public static IEnumerable Select( [MethodImpl(MethodImplOptions.AggressiveInlining)] private static IEnumerable SelectImplementation(Func selector, Iterator iterator) { - // In Native AOT, GVMs with Value types are not trim friendly. - // If the option is enabled, we don't call the GVM for value types. We don't do the - // same for reference types because reference types are trim friendly with GVMs - // and it is preferable to call the GVM as it allows the select implementation to be - // specialized. + // With native AOT, calling into the `Select` generic virtual method results in NxM + // expansion of native code. If the option is enabled, we don't call the generic virtual + // for value types. We don't do the same for reference types because reference type + // expansion can happen lazily at runtime and the AOT compiler does postpone it (we + // don't need more code, just more data structures describing the new types). if (/*IteratorOptions.ValueTypeTrimFriendlySelect && */typeof(TResult).IsValueType) { #if OPTIMIZE_FOR_SIZE From 67674d5a4015b5b5ef0224d1b7dbe7fa7ace6daa Mon Sep 17 00:00:00 2001 From: Keegan Caruso Date: Tue, 26 Nov 2024 18:42:23 -0800 Subject: [PATCH 4/6] PR Feedback - Don't set feature flag by default for trimming - Move option to Enumerable - Fix error in OfType.SpeedOpt --- src/libraries/System.Linq/src/System.Linq.csproj | 1 - .../System.Linq/src/System/Linq/Enumerable.cs | 4 ++++ .../src/System/Linq/IteratorOptions.cs | 16 ---------------- .../src/System/Linq/OfType.SpeedOpt.cs | 2 +- .../System.Linq/src/System/Linq/Select.cs | 2 +- .../build/Microsoft.NET.ILLink.targets | 1 - 6 files changed, 6 insertions(+), 20 deletions(-) delete mode 100644 src/libraries/System.Linq/src/System/Linq/IteratorOptions.cs diff --git a/src/libraries/System.Linq/src/System.Linq.csproj b/src/libraries/System.Linq/src/System.Linq.csproj index 32500dfa5ebd6..79c10444f489b 100644 --- a/src/libraries/System.Linq/src/System.Linq.csproj +++ b/src/libraries/System.Linq/src/System.Linq.csproj @@ -64,7 +64,6 @@ - diff --git a/src/libraries/System.Linq/src/System/Linq/Enumerable.cs b/src/libraries/System.Linq/src/System/Linq/Enumerable.cs index fbdf9a12a28b6..cc508f8446017 100644 --- a/src/libraries/System.Linq/src/System/Linq/Enumerable.cs +++ b/src/libraries/System.Linq/src/System/Linq/Enumerable.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; @@ -63,5 +64,8 @@ internal static bool TryGetSpan(this IEnumerable source, out R return result; } + + [FeatureSwitchDefinition("System.Linq.Enumerable.ValueTypeTrimFriendlySelect")] + internal static bool ValueTypeTrimFriendlySelect { get; } = AppContext.TryGetSwitch("System.Linq.Enumerable.ValueTypeTrimFriendlySelect", out bool isEnabled) ? isEnabled : false; } } diff --git a/src/libraries/System.Linq/src/System/Linq/IteratorOptions.cs b/src/libraries/System.Linq/src/System/Linq/IteratorOptions.cs deleted file mode 100644 index 226b84338337a..0000000000000 --- a/src/libraries/System.Linq/src/System/Linq/IteratorOptions.cs +++ /dev/null @@ -1,16 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Diagnostics.CodeAnalysis; - -namespace System.Linq -{ - /// - /// Defines options for how iterators behave. - /// - internal static class IteratorOptions - { - [FeatureSwitchDefinition("System.Linq.ValueTypeTrimFriendlySelect")] - public static bool ValueTypeTrimFriendlySelect { get; } = AppContext.TryGetSwitch("System.Linq.ValueTypeTrimFriendlySelect", out bool isEnabled) ? isEnabled : true; - } -} diff --git a/src/libraries/System.Linq/src/System/Linq/OfType.SpeedOpt.cs b/src/libraries/System.Linq/src/System/Linq/OfType.SpeedOpt.cs index bbe3810251830..f059542b72bd2 100644 --- a/src/libraries/System.Linq/src/System/Linq/OfType.SpeedOpt.cs +++ b/src/libraries/System.Linq/src/System/Linq/OfType.SpeedOpt.cs @@ -172,7 +172,7 @@ public override IEnumerable Select(Func s new IEnumerableWhereSelectIterator(objectSource, isTResult, localSelector); } - return SelectImplementation(selector, this); + return new IteratorSelectIterator(this, selector); } } } diff --git a/src/libraries/System.Linq/src/System/Linq/Select.cs b/src/libraries/System.Linq/src/System/Linq/Select.cs index fac0978314cb6..79700032a69d7 100644 --- a/src/libraries/System.Linq/src/System/Linq/Select.cs +++ b/src/libraries/System.Linq/src/System/Linq/Select.cs @@ -60,7 +60,7 @@ private static IEnumerable SelectImplementation(Func< // for value types. We don't do the same for reference types because reference type // expansion can happen lazily at runtime and the AOT compiler does postpone it (we // don't need more code, just more data structures describing the new types). - if (/*IteratorOptions.ValueTypeTrimFriendlySelect && */typeof(TResult).IsValueType) + if (/*ValueTypeTrimFriendlySelect &&*/ typeof(TResult).IsValueType) { #if OPTIMIZE_FOR_SIZE return new IEnumerableSelectIterator(iterator, selector); diff --git a/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets b/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets index eff4d7bef6f30..3fdf083a3beda 100644 --- a/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets +++ b/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets @@ -38,7 +38,6 @@ Copyright (c) .NET Foundation. All rights reserved. false false false - true false false From d96f35a29985925af0ee78914d9aa7d89085612b Mon Sep 17 00:00:00 2001 From: Keegan Caruso Date: Tue, 26 Nov 2024 18:43:09 -0800 Subject: [PATCH 5/6] temporary: enable option by default for testing --- src/libraries/System.Linq/src/System/Linq/Enumerable.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Linq/src/System/Linq/Enumerable.cs b/src/libraries/System.Linq/src/System/Linq/Enumerable.cs index cc508f8446017..6b591259337a5 100644 --- a/src/libraries/System.Linq/src/System/Linq/Enumerable.cs +++ b/src/libraries/System.Linq/src/System/Linq/Enumerable.cs @@ -66,6 +66,6 @@ internal static bool TryGetSpan(this IEnumerable source, out R } [FeatureSwitchDefinition("System.Linq.Enumerable.ValueTypeTrimFriendlySelect")] - internal static bool ValueTypeTrimFriendlySelect { get; } = AppContext.TryGetSwitch("System.Linq.Enumerable.ValueTypeTrimFriendlySelect", out bool isEnabled) ? isEnabled : false; + internal static bool ValueTypeTrimFriendlySelect { get; } = AppContext.TryGetSwitch("System.Linq.Enumerable.ValueTypeTrimFriendlySelect", out bool isEnabled) ? isEnabled : true; } } From 41d7467ed7d8418f6498323fd69d42ffa88eb76c Mon Sep 17 00:00:00 2001 From: Keegan Caruso Date: Wed, 27 Nov 2024 11:06:06 -0800 Subject: [PATCH 6/6] Use remote executor for tests using appcontext --- .../System.Linq/src/System/Linq/Select.cs | 2 +- .../System.Linq/tests/SkipWhileTests.cs | 32 +++++++++++++++---- .../tests/System.Linq.Tests.csproj | 1 + 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Linq/src/System/Linq/Select.cs b/src/libraries/System.Linq/src/System/Linq/Select.cs index 79700032a69d7..db9adc9874030 100644 --- a/src/libraries/System.Linq/src/System/Linq/Select.cs +++ b/src/libraries/System.Linq/src/System/Linq/Select.cs @@ -60,7 +60,7 @@ private static IEnumerable SelectImplementation(Func< // for value types. We don't do the same for reference types because reference type // expansion can happen lazily at runtime and the AOT compiler does postpone it (we // don't need more code, just more data structures describing the new types). - if (/*ValueTypeTrimFriendlySelect &&*/ typeof(TResult).IsValueType) + if (ValueTypeTrimFriendlySelect && typeof(TResult).IsValueType) { #if OPTIMIZE_FOR_SIZE return new IEnumerableSelectIterator(iterator, selector); diff --git a/src/libraries/System.Linq/tests/SkipWhileTests.cs b/src/libraries/System.Linq/tests/SkipWhileTests.cs index a13a686cef3f3..5884d9c7ce3c1 100644 --- a/src/libraries/System.Linq/tests/SkipWhileTests.cs +++ b/src/libraries/System.Linq/tests/SkipWhileTests.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; +using Microsoft.DotNet.RemoteExecutor; using Xunit; namespace System.Linq.Tests @@ -62,14 +63,33 @@ public void RunOnce() Assert.Equal(Enumerable.Range(10, 10), Enumerable.Range(0, 20).RunOnce().SkipWhile((i, idx) => idx < 10)); } - [Fact] - public void SkipErrorWhenSourceErrors() + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + public static void SkipErrorWhenSourceErrors_TrimFriendlySelectTrue() { - var source = NumberRangeGuaranteedNotCollectionType(-2, 5).Select(i => (decimal)i).Select(m => 1 / m).Skip(4); - using(var en = source.GetEnumerator()) + RemoteExecutor.Invoke(() => { - Assert.Throws(() => en.MoveNext()); - } + AppContext.SetSwitch("System.Linq.Enumerable.ValueTypeTrimFriendlySelect", true); + + var source = NumberRangeGuaranteedNotCollectionType(-2, 5).Select(i => (decimal)i).Select(m => 1 / m).Skip(4); + var valuesFromEnumerable = source.ToList(); + List expectedValues = [(decimal)1/2]; + Assert.Equal(expectedValues, valuesFromEnumerable); + }).Dispose(); + } + + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + public void SkipErrorWhenSourceErrors_TrimFriendlySelectFalse() + { + RemoteExecutor.Invoke(() => + { + AppContext.SetSwitch("System.Linq.Enumerable.ValueTypeTrimFriendlySelect", false); + + var source = NumberRangeGuaranteedNotCollectionType(-2, 5).Select(i => (decimal)i).Select(m => 1 / m).Skip(4); + using (var en = source.GetEnumerator()) + { + Assert.Throws(() => en.MoveNext()); + } + }).Dispose(); } [Fact] diff --git a/src/libraries/System.Linq/tests/System.Linq.Tests.csproj b/src/libraries/System.Linq/tests/System.Linq.Tests.csproj index 1e4692999a174..ac0ef5e696463 100644 --- a/src/libraries/System.Linq/tests/System.Linq.Tests.csproj +++ b/src/libraries/System.Linq/tests/System.Linq.Tests.csproj @@ -1,5 +1,6 @@ + true $(NetCoreAppCurrent) true