Optimize single spread of IEnumerable to array #75847
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| private BoundExpression CreateAndPopulateArray(BoundCollectionExpression node, ArrayTypeSymbol arrayType) | ||
| /// <summary>Attempt to optimize conversion of a single-spread collection expr to array, even if the spread length is not known.</summary> | ||
| private BoundExpression? TryOptimizeSingleSpreadToArray(BoundCollectionExpression node, ArrayTypeSymbol arrayType) |
There was a problem hiding this comment.
There's now 3 possible optimizations this method will try, in order:
- List.ToArray if the spread value is a list
- IEnumerable.ToArray if our checks say we should convert the spread value in this way
- ReadOnlySpan.ToArray if we can convert the spread value to ReadOnlySpan
In my opinion this is the best order to try these, in part because IEnumerable.ToArray tends to produce less IL than the span conversions.
There was a problem hiding this comment.
Consider capturing this information in a comment
| int[] arr1 = [..arr, ..arr]; | ||
| arr1.Report(); | ||
| } | ||
| public static object[] M(string[] arr) => [..arr]; |
There was a problem hiding this comment.
Perhaps test the following cases:
string[] x = ["a"];
M1(x);
M2(x);
static Span<object> M1(string[] arr) => [..arr];
static Span<object> M2(object[] arr) => [..arr];
| using System; | ||
|
|
||
| C.M1(["a"]); | ||
| C.M2(["b"]); |
There was a problem hiding this comment.
C.M2(["b"]) creates an object[] rather than a string[]. Consider rewriting as:
string[] s = ["a"];
C.M1(s);
C.M2(s);
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@RikkiGibson It looks like #71755 is going to be partially addressed, but this PR is closing it. Do we have a different issue tracking the work for a more general case? |
| { | ||
| var rewrittenSpreadExpression = VisitExpression(spreadExpression); | ||
| return _factory.Call(rewrittenSpreadExpression, listToArrayMethod.AsMember((NamedTypeSymbol)spreadExpression.Type!)); | ||
| } | ||
|
|
||
| if (TryGetSpanConversion(spreadExpression.Type, writableOnly: false, out var asSpanMethod)) | ||
| // See if 'Enumerable.ToArray<T>(IEnumerable<T>)' will work, possibly due to a covariant conversion on the spread value. | ||
| if (IsConvertibleToObject(arrayType.ElementType) |
| // conversion to 'object' will fail if, for example, 'arrayType.ElementType' is a pointer. | ||
| var useSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded; | ||
| if (_compilation.Conversions.ClassifyConversionFromType(source: arrayType.ElementType, destination: _compilation.GetSpecialType(SpecialType.System_Object), isChecked: false, ref useSiteInfo).IsImplicit) | ||
| if (IsConvertibleToObject(arrayType.ElementType)) |
There was a problem hiding this comment.
What is the purpose of this check? Are we trying to confirm whether the element type is a valid type argument for a span type? If so, the name of the method feels misleading, and its implementation draws a conclusion from seemingly unrelated check (it is not obvious if it could filter more or less than we want to in some cases). Consider renaming the method and adjusting its implementation to perform a direct check, either by using one of the CheckConstraints helpers, or by simply checking !elementType.IsPointerOrFunctionPointer() && !elementType.IsRestrictedType()).
|
Done with review pass (commit 9) |
Yes, I think #75863 represents the extent of the further improvement we could realistically make to the general case of #71755. |
Closes #71296
Closes #71755
Some of these issues also discuss optimization in cases when multiple spreads of unknown length are present in the collection-expr, such as
[..e1, e2, ..e3]and so on. I am thinking we should just handle the single spread case[..e]for now as this is the source of majority of complaints. This is now handled by simply callingSystem.Linq.Enumerable.ToArray<T>(IEnumerable<T>)in most cases when the spread value is convertible to IEnumerable.