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.

Sign up for GitHub

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

Jump to bottom

Optimize single spread collection expression for List<T> #74769

Merged
merged 21 commits into from
Sep 3, 2024
Merged

Optimize single spread collection expression for List<T> #74769

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
b3570d7
Move list check up
DoctorKrolic Aug 15, 2024
a973cc8
Add well-known members for `Enumerable.ToList` and `new List(IEnumera…
DoctorKrolic Aug 15, 2024
ed9f0c6
The optimization itself
DoctorKrolic Aug 15, 2024
bc71192
Turn local function into instance method
DoctorKrolic Aug 20, 2024
c7a3f51
Check conversion kind
DoctorKrolic Aug 20, 2024
dfa95e5
Test non-generic enumerable
DoctorKrolic Aug 20, 2024
6895189
Add test with public and explicit `GetEnumerator`
DoctorKrolic Aug 20, 2024
0a66f09
Test generic element conversions
DoctorKrolic Aug 20, 2024
7e5b188
Remove constructor fallback
DoctorKrolic Aug 21, 2024
bd468b4
Merge branch 'dotnet:main' into collection-expressions-to-list
DoctorKrolic Aug 21, 2024
a6e8878
Extract common logic
DoctorKrolic Aug 23, 2024
05ffe61
Change signature
DoctorKrolic Aug 24, 2024
95274af
Asserts
DoctorKrolic Aug 24, 2024
c53a218
Reorder
DoctorKrolic Aug 24, 2024
ae1c2be
Add test
DoctorKrolic Aug 25, 2024
2c8ac16
Merge branch 'dotnet:main' into collection-expressions-to-list
DoctorKrolic Aug 28, 2024
4125d52
Redundant parameter
DoctorKrolic Aug 29, 2024
23d22b2
Rewrite spread expression
DoctorKrolic Aug 29, 2024
2234846
Merge `main` and make necessary changes
DoctorKrolic Aug 30, 2024
4c710c1
Simplify
DoctorKrolic Aug 31, 2024
e817f62
Add test
DoctorKrolic Aug 31, 2024
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
100 changes: 68 additions & 32 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,17 @@ private BoundExpression RewriteCollectionExpressionConversion(Conversion convers
switch (collectionTypeKind)
{
case CollectionExpressionTypeKind.ImplementsIEnumerable:
if (useListOptimization(_compilation, node, out var listElementType))
if (ConversionsBase.IsSpanOrListType(_compilation, node.Type, WellKnownType.System_Collections_Generic_List_T, out var listElementType))
{
return CreateAndPopulateList(node, listElementType, node.Elements.SelectAsArray(static (element, node) => unwrapListElement(node, element), node));
if (TryRewriteSingleElementSpreadToList(node, listElementType, out var result))
{
return result;
}

if (useListOptimization(_compilation, node))
{
return CreateAndPopulateList(node, listElementType, node.Elements.SelectAsArray(static (element, node) => unwrapListElement(node, element), node));
}
}
return VisitCollectionInitializerCollectionExpression(node, node.Type);
case CollectionExpressionTypeKind.Array:
Expand Down Expand Up @@ -88,12 +96,8 @@ private BoundExpression RewriteCollectionExpressionConversion(Conversion convers

// If the collection type is List<T> and items are added using the expected List<T>.Add(T) method,
// then construction can be optimized to use CollectionsMarshal methods.
static bool useListOptimization(CSharpCompilation compilation, BoundCollectionExpression node, out TypeWithAnnotations elementType)
static bool useListOptimization(CSharpCompilation compilation, BoundCollectionExpression node)
{
if (!ConversionsBase.IsSpanOrListType(compilation, node.Type, WellKnownType.System_Collections_Generic_List_T, out elementType))
{
return false;
}
var elements = node.Elements;
if (elements.Length == 0)
{
Expand Down Expand Up @@ -151,6 +155,59 @@ static BoundNode unwrapListElement(BoundCollectionExpression node, BoundNode ele
}
}

// If we have something like `List<int> l = [.. someEnumerable]`
// try rewrite it using `Enumerable.ToList` member if possible
private bool TryRewriteSingleElementSpreadToList(BoundCollectionExpression node, TypeWithAnnotations listElementType, [NotNullWhen(true)] out BoundExpression? result)
{
result = null;

if (node.Elements is not [BoundCollectionExpressionSpreadElement singleSpread])
{
return false;
}
Comment on lines +164 to +167
Copy link
Contributor

@alrz alrz Aug 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wish collection expressions were implemented using a chain of nodes based on how the codegen would work, then this kind of optimization would be a lot simpler to implement IMO, for example [a, b, .. array, .. list, etc] yields [ Block(a, b), SpreadArray(array), SpreadList(list), Element(etc) ] and then pattern-match on that, this would separate the initial analysis and the final codegen. The structure would look a lot like how string concat works but a little more involved to aid codegen.


if (!ShouldUseAddRangeOrToListMethod(singleSpread.Expression.Type!, singleSpread.EnumeratorInfoOpt?.GetEnumeratorInfo.Method, listElementType))
DoctorKrolic marked this conversation as resolved.
Show resolved Hide resolved
{
return false;
}

if (TryGetWellKnownTypeMember(node.Syntax, WellKnownMember.System_Linq_Enumerable__ToList, out MethodSymbol? toListGeneric, isOptional: true))
{
var toListOfElementType = toListGeneric.Construct([listElementType]);
result = _factory.Call(receiver: null, toListOfElementType, singleSpread.Expression);
return true;
}

return false;
}

private bool ShouldUseAddRangeOrToListMethod(TypeSymbol spreadType, MethodSymbol? getEnumeratorMethod, TypeWithAnnotations elementType)
{
var iEnumerableOfTType = _compilation.GetSpecialType(SpecialType.System_Collections_Generic_IEnumerable_T);
var iEnumerableOfElementType = iEnumerableOfTType.Construct([elementType]);

var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;

var conversion = _compilation.Conversions.ClassifyImplicitConversionFromType(spreadType, iEnumerableOfElementType, ref discardedUseSiteInfo);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we maybe use the actual use site info for thisand report it if we take the optimzal code path an the end? Unline the ICollection<T> check, the resulting conversion is kinda used in the final codegn, although implicitly

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the original code for the AddRange case was reporting the use-site diagnostic, but I think it's fine to drop it. The code we generate does not actually use this type in any way, it's just a hint for us to decide if AddRange/ToList is likely to be efficient.

Copy link
Member

Choose a reason for hiding this comment

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

var conversion = _compilation.Conversions.ClassifyImplicitConversionFromType(spreadType, iEnumerableOfElementType, ref discardedUseSiteInfo);

Let's use the code from tryOptimizeSpreadElement which checks for ICollection<T>, then checks the conversion to addRangeMethod.Parameters[0].Type rather than the conversion to IEnumerable<T> directly. But perhaps we should use ClassifyImplicitConversionFromType() rather than ClassifyConversionFromType() for the parameter conversion check. Also, the comment in the ICollection<T> check from tryOptimizeSpreadElement would be good to keep. Obviously, the references to AddRange should be updated though.

Copy link
Member

Choose a reason for hiding this comment

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

Also, the ICollection<T> check in tryOptimizeSpreadElement used spreadElement.EnumeratorInfo.CollectionType and spreadElement.EnumeratorInfo.ElementType. With this change, we are using spreadElement.Expression.Type and the element type from the target type. Is there a difference between those pairs of types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a difference between those pairs of types?

I didn't implement logic in the binding layer, that fills in EnumeratorInfoOpt, but from the sanity point of view they should be the same. I preffer avoiding EnumeratorInfoOpt since it can be null. Or maybe we can assume, that at the point of lowering we always have it and drop additional null checks?

if (conversion.Kind is not (ConversionKind.Identity or ConversionKind.ImplicitReference))
{
return false;
}

if (getEnumeratorMethod?.ReturnType.IsValueType == true)
{
var iCollectionOfTType = _compilation.GetSpecialType(SpecialType.System_Collections_Generic_ICollection_T);
var iCollectionOfElementType = iCollectionOfTType.Construct([elementType]);

if (!spreadType.ImplementsInterface(iCollectionOfElementType, ref discardedUseSiteInfo))
DoctorKrolic marked this conversation as resolved.
Show resolved Hide resolved
{
return false;
}
}

return true;
DoctorKrolic marked this conversation as resolved.
Show resolved Hide resolved
}

private static bool CanOptimizeSingleSpreadAsCollectionBuilderArgument(BoundCollectionExpression node, [NotNullWhen(true)] out BoundExpression? spreadExpression)
{
spreadExpression = null;
Expand Down Expand Up @@ -1067,34 +1124,13 @@ private BoundExpression CreateAndPopulateList(BoundCollectionExpression node, Ty
if (addRangeMethod is null)
return false;

if (spreadElement.EnumeratorInfoOpt is { } enumeratorInfo)
{
var iCollectionOfTType = _compilation.GetSpecialType(SpecialType.System_Collections_Generic_ICollection_T);
var iCollectionOfElementType = iCollectionOfTType.Construct(enumeratorInfo.ElementType);
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;

// If collection has a struct enumerator but doesn't implement ICollection<T>
// then manual `foreach` is always more efficient then using `AddRange` method
if (enumeratorInfo.GetEnumeratorInfo.Method.ReturnType.IsValueType &&
!enumeratorInfo.CollectionType.ImplementsInterface(iCollectionOfElementType, ref discardedUseSiteInfo))
{
return false;
}
}

var type = rewrittenSpreadOperand.Type!;

var useSiteInfo = GetNewCompoundUseSiteInfo();
var conversion = _compilation.Conversions.ClassifyConversionFromType(type, addRangeMethod.Parameters[0].Type, isChecked: false, ref useSiteInfo);
_diagnostics.Add(rewrittenSpreadOperand.Syntax, useSiteInfo);
if (conversion.IsIdentity || (conversion.IsImplicit && conversion.IsReference))
if (!ShouldUseAddRangeOrToListMethod(rewrittenSpreadOperand.Type!, spreadElement.EnumeratorInfoOpt?.GetEnumeratorInfo.Method, elementType))
DoctorKrolic marked this conversation as resolved.
Show resolved Hide resolved
{
conversion.MarkUnderlyingConversionsCheckedRecursive();
sideEffects.Add(_factory.Call(listTemp, addRangeMethod, rewrittenSpreadOperand));
return true;
return false;
}

return false;
sideEffects.Add(_factory.Call(listTemp, addRangeMethod, rewrittenSpreadOperand));
return true;
});
}

Expand Down
Loading
Loading