Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use a GVM in Linq Select with NAOT by default #109978

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libraries/System.Linq/src/System.Linq.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
<Compile Include="System\Linq\Index.cs" />
<Compile Include="System\Linq\Intersect.cs" />
<Compile Include="System\Linq\Iterator.cs" />
<Compile Include="System\Linq\IteratorOptions.cs" />
<Compile Include="System\Linq\Join.cs" />
<Compile Include="System\Linq\Last.cs" />
<Compile Include="System\Linq\Lookup.cs" />
Expand Down
16 changes: 16 additions & 0 deletions src/libraries/System.Linq/src/System/Linq/IteratorOptions.cs
Original file line number Diff line number Diff line change
@@ -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
{
/// <summary>
/// Defines options for how iterators behave.
/// </summary>
internal static class IteratorOptions
{
[FeatureSwitchDefinition("System.Linq.ValueTypeTrimFriendlySelect")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to pick feature switch names that could match the name of a public property (if we ever decided to make it public) - so maybe put this in the Enumerable class?

public static bool ValueTypeTrimFriendlySelect { get; } = AppContext.TryGetSwitch("System.Linq.ValueTypeTrimFriendlySelect", out bool isEnabled) ? isEnabled : true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public override IEnumerable<TResult2> Select<TResult2>(Func<TResult, TResult2> s
new IEnumerableWhereSelectIterator<object, TResult2>(objectSource, isTResult, localSelector);
}

return base.Select(selector);
return SelectImplementation(selector, this);
}
}
}
Expand Down
23 changes: 22 additions & 1 deletion src/libraries/System.Linq/src/System/Linq/Select.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -25,7 +26,7 @@ public static IEnumerable<TResult> Select<TSource, TResult>(

if (source is Iterator<TSource> iterator)
{
return iterator.Select(selector);
return SelectImplementation(selector, iterator);
}

if (source is IList<TSource> ilist)
Expand All @@ -51,6 +52,26 @@ public static IEnumerable<TResult> Select<TSource, TResult>(
return new IEnumerableSelectIterator<TSource, TResult>(source, selector);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static IEnumerable<TResult> SelectImplementation<TSource, TResult>(Func<TSource, TResult> selector, Iterator<TSource> 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.
Comment on lines +58 to +62
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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
return new IEnumerableSelectIterator<TSource, TResult>(iterator, selector);
#else
return new IteratorSelectIterator<TSource, TResult>(iterator, selector);
#endif
}

return iterator.Select(selector);
}

public static IEnumerable<TResult> Select<TSource, TResult>(this IEnumerable<TSource> source, Func<TSource, int, TResult> selector)
{
if (source is null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<EnableUnsafeBinaryFormatterSerialization Condition="'$(EnableUnsafeBinaryFormatterSerialization)' == ''">false</EnableUnsafeBinaryFormatterSerialization>
<EnableUnsafeUTF7Encoding Condition="'$(EnableUnsafeUTF7Encoding)' == ''">false</EnableUnsafeUTF7Encoding>
<BuiltInComInteropSupport Condition="'$(BuiltInComInteropSupport)' == ''">false</BuiltInComInteropSupport>
<ValueTypeTrimFriendlySelect Condition="'$(ValueTypeTrimFriendlySelect)' == ''">true</ValueTypeTrimFriendlySelect>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any measurements how this affects PublishTrimmed? This enables it if PublishTrimmed is true, I wonder if it should set defaults based on PublishAot instead.

<!-- GeneratedComInterface/ComImport interop requires built-in COM -->
<EnableGeneratedComInterfaceComImportInterop Condition="'$(BuiltInComInteropSupport)' == 'false'">false</EnableGeneratedComInterfaceComImportInterop>
<EnableGeneratedComInterfaceComImportInterop Condition="'$(EnableGeneratedComInterfaceComImportInterop)' == ''">false</EnableGeneratedComInterfaceComImportInterop>
Expand Down
Loading