Collection expression spread optimization #71195

RikkiGibson · 2023-12-09T04:07:24Z

Closes #70656
Closes #69988
Closes #69626 (I didn't see which types expose a well-known AddRange(ReadOnlySpan) though.)

Convert a collection-expr of the form [..expr] to expr.ToArray() when possible.
- When expr is List<T>, using List<T>.ToArray()
- Otherwise by converting expr to Span/ReadOnlySpan then calling ToArray
Convert a collection-expr of the form [elems, ..expr] to use expr.CopyTo(dest) when possible.
- Converting dest to Span<T>
- Converting expr to Span<T> or ReadOnlySpan<T>
Convert a collection-expr of the form [elems, ..expr] to dest.AddRange(expr) for List<T> when CollectionsMarshal is not available
- Converting expr to IEnumerable<T> (only if it is an implicit reference conversion)

Perf data for these various strategies can be found here:
(for arrays) #70656 (comment)
(for lists) #70656 (comment)

CyrusNajmabadi · 2023-12-09T04:56:54Z

This is awesome.

I think it would be good to have a simple doc that lists the patterns we optimize, and the version of the compiler (maybe) that introduced the optimization. It would be good for people who want to know when this stuff does or doesn't happen.

…ead-optimization

RikkiGibson · 2023-12-12T00:43:03Z

It looks like a change in the base branch is preventing optimized codegen in 7 tests in CollectionExpressionTests, and I forgot to update a VB WellKnownMembers test. I'll look into it tomorrow.

…ead-optimization

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

cston · 2023-12-15T17:54:40Z

        verifier.VerifyIL("C.Main", """

Should this verify C.<Main>d__1.System.Runtime.CompilerServices.IAsyncStateMachine.MoveNext() instead? #Closed

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:27100 in cce39d0. [](commit_id = cce39d0, deletion_comment = False)

333fred

Done review pass

333fred · 2023-12-15T21:01:25Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+                    }
+                }
+
+                MethodSymbol? tryGetToArrayMethod(TypeSymbol spreadTypeOriginalDefinition, WellKnownType wellKnownType, WellKnownMember wellKnownMember)


This isn't really a try-get in traditional form, and it feels like the code that uses it is a bit convoluted because of it. Consider refactoring to be a standard tryGet with an out parameter.

333fred · 2023-12-15T21:02:53Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

@@ -599,6 +636,20 @@ private BoundExpression CreateAndPopulateArray(BoundCollectionExpression node, A
                            _factory.Binary(BinaryOperatorKind.Addition, indexTemp.Type, indexTemp, _factory.Literal(1)),
                            isRef: false,
                            indexTemp.Type));
+                },
+                (sideEffects, arrayTemp, spreadElement, rewrittenSpreadOperand) =>


Consider naming these delegate parameters, especially now that there are multiple of them.

Also, please spell out the types of these parameters. Same goes for the other similar delegates below.

I wasn't sure what you meant by name the delegate parameters. Were you suggesting that I declare a named delegate type which has these parameter names?

No, I mean specify the parameter name for each of these delegate arguments. What is the parameter this delegate is being passed to?

333fred · 2023-12-15T21:06:17Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+        /// Returns true if type is convertible to Span or ReadOnlySpan.
+        /// If non-identity conversion, also returns a non-null asSpanMethod.
+        /// </summary>
+        private bool TryGetSpanConversion(TypeSymbol type, out MethodSymbol? asSpanMethod)


Let's rearrange this method to simplify things. First, check to see if it's the array case. The rest of the cases depend on it being a named type symbol, so you can do that check once and bail if false, rather than type-checking against NamedTypeSymbol 3 times.

333fred · 2023-12-15T21:12:27Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            return false;
+        }
+
+        private BoundExpression? TryConvertToSpanOrReadOnlySpan(BoundExpression expression)


It feels like this is another method that would be better served by following the standard TryGet pattern.

333fred · 2023-12-15T21:16:26Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+                    && _factory.WellKnownMethod(getLengthMember, isOptional: true) is { } getLengthMethod
+                    && _factory.WellKnownMethod(copyToMember, isOptional: true) is { } copyToMethod)
+                {
+                    return (getLengthMethod!, copyToMethod!);


Why do we need suppressions on this line?

333fred · 2023-12-15T21:31:24Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            if (type is ArrayTypeSymbol { IsSZArray: true } arrayType
+                && _factory.WellKnownMethod(WellKnownMember.System_Span_T__ctor_Array, isOptional: true) is { } spanCtorArray)
+            {
+                asSpanMethod = spanCtorArray.AsMember(spanCtorArray.ContainingType.Construct(arrayType.ElementType));


What if the element type is something that can't be a valid type parameter, like a pointer? Do we have a test covering that?

333fred · 2023-12-15T23:15:30Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            if (type is ArrayTypeSymbol { IsSZArray: true } arrayType
+                && _factory.WellKnownMethod(WellKnownMember.System_Span_T__ctor_Array, isOptional: true) is { } spanCtorArray)
+            {
+                asSpanMethod = spanCtorArray.AsMember(spanCtorArray.ContainingType.Construct(arrayType.ElementType));


How do we know the element type is always going to be a valid type argument? Can we assert something?

I would be fine with asserting, for example, that element type is implicitly convertible to object.

This was a faulty assumption. The conversion to object is now checked in retail builds and if it isn't what we expect then we don't use either the ToArray or CopyTo optimization paths.

Do we have a test that hits this? (It looks like the existing tests were unchanged, other than setting the target framework.)

The new tests require the object conversion check in order to work properly.

333fred · 2023-12-15T23:17:31Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

@@ -599,6 +637,19 @@ private BoundExpression CreateAndPopulateArray(BoundCollectionExpression node, A
                            _factory.Binary(BinaryOperatorKind.Addition, indexTemp.Type, indexTemp, _factory.Literal(1)),
                            isRef: false,
                            indexTemp.Type));
+                },
+                (ArrayBuilder<BoundExpression> sideEffects, BoundExpression arrayTemp, BoundCollectionExpressionSpreadElement spreadElement, BoundExpression rewrittenSpreadOperand) =>


Still missing tryOptimizeSpreadElement: naming.

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 9, 2023

Collection expression spread optimization

511526c

This was referenced Dec 11, 2023

Consider preferring AddRange over CopyTo for collection-expressions of List type #71216

Closed

Use ToList for collection-exprs of a single spread #71217

Closed

RikkiGibson force-pushed the collection-expr-spread-optimization branch from 60c4efa to 511526c Compare December 11, 2023 23:38

RikkiGibson marked this pull request as ready for review December 11, 2023 23:38

RikkiGibson requested a review from a team as a code owner December 11, 2023 23:38

Merge remote-tracking branch 'upstream/main' into collection-expr-spr…

76cd2bb

…ead-optimization

RikkiGibson added 4 commits December 12, 2023 13:23

cleanup

fd3145e

Merge remote-tracking branch 'upstream/main' into collection-expr-spr…

1a3ed67

…ead-optimization

Adjust test baseline to reflect intention of test

00a17f6

Fix WellKnownTypeValidationTests

d295100

cston reviewed Dec 12, 2023

View reviewed changes

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs Outdated Show resolved Hide resolved

cston reviewed Dec 12, 2023

View reviewed changes

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs Show resolved Hide resolved

cston reviewed Dec 12, 2023

View reviewed changes

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs Show resolved Hide resolved

RikkiGibson added 2 commits December 12, 2023 14:52

address feedback and fixup tests

853f8aa

Fix MissingSpecialMember tests

38ee84e

cston reviewed Dec 12, 2023

View reviewed changes

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs Outdated Show resolved Hide resolved

cston reviewed Dec 12, 2023

View reviewed changes

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs Outdated Show resolved Hide resolved

cston reviewed Dec 13, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs Outdated Show resolved Hide resolved

cston reviewed Dec 13, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs Outdated Show resolved Hide resolved

cston reviewed Dec 13, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs Outdated Show resolved Hide resolved

cston reviewed Dec 13, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs Outdated Show resolved Hide resolved

cston reviewed Dec 13, 2023

View reviewed changes

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs Show resolved Hide resolved

cston reviewed Dec 13, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs Outdated Show resolved Hide resolved

cston reviewed Dec 13, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs Outdated Show resolved Hide resolved

cston reviewed Dec 13, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs Outdated Show resolved Hide resolved

RikkiGibson mentioned this pull request Dec 14, 2023

Store the dest collection as span to a temp for CopyTo optimization #71270

Open

Get rid of temp for destination span. Adjust test baselines

501d5b1

This comment was marked as resolved.

Sign in to view

RikkiGibson added 3 commits December 14, 2023 21:43

Address more feedback

36fb608

More tests. Address TODOs.

dc7ef72

Fix raw string indenting

c93244e

RikkiGibson requested a review from cston December 15, 2023 06:11

Skip verification on a few more tests

cce39d0

cston approved these changes Dec 15, 2023

View reviewed changes

RikkiGibson requested a review from a team December 15, 2023 18:57

333fred reviewed Dec 15, 2023

View reviewed changes

Address feedback

8625ac3

333fred reviewed Dec 15, 2023

View reviewed changes

RikkiGibson added 2 commits December 15, 2023 15:32

address feedback

da6ad13

Adjust assertion

a071343

RikkiGibson requested a review from 333fred December 15, 2023 23:57

cston approved these changes Dec 16, 2023

View reviewed changes

Add targetFramework to pointer tests. Fix impl.

1b1ad03

333fred approved these changes Dec 16, 2023

View reviewed changes

RikkiGibson merged commit 6a1cfc2 into dotnet:main Dec 16, 2023
25 checks passed

ghost added this to the Next milestone Dec 16, 2023

RikkiGibson deleted the collection-expr-spread-optimization branch December 16, 2023 02:25

This was referenced Dec 16, 2023

Poor codegen for collection expression spread (ROS<T> -> ImmutableArray<T>) #71292

Closed

Poor codegen for collection expression spread (IEnumerable<T> -> ImmutableArray<T>) #71296

Closed

allisonchou mentioned this pull request Dec 18, 2023

[Automated] PRs inserted in VS build main-34418.87 #71331

Closed

Cosifne modified the milestones: Next, 17.9 P3 Jan 9, 2024

RikkiGibson mentioned this pull request Mar 6, 2024

Use the List(int) constructor if possible when lowering collection expressions #72427

Merged

RikkiGibson mentioned this pull request Apr 22, 2024

Optimize codegen for collections expression of single spread of ReadOnlySpan for collection builder emit strategy #73102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection expression spread optimization #71195

Collection expression spread optimization #71195

RikkiGibson commented Dec 9, 2023 •

edited

Loading

CyrusNajmabadi commented Dec 9, 2023

RikkiGibson commented Dec 12, 2023

This comment was marked as resolved.

cston commented Dec 15, 2023 •

edited

Loading

333fred left a comment

333fred Dec 15, 2023

333fred Dec 15, 2023

333fred Dec 15, 2023

RikkiGibson Dec 15, 2023

333fred Dec 15, 2023

333fred Dec 15, 2023

333fred Dec 15, 2023

333fred Dec 15, 2023

333fred Dec 15, 2023

333fred Dec 15, 2023

RikkiGibson Dec 15, 2023

RikkiGibson Dec 16, 2023

cston Dec 16, 2023

RikkiGibson Dec 16, 2023

333fred Dec 15, 2023

Collection expression spread optimization #71195

Collection expression spread optimization #71195

Conversation

RikkiGibson commented Dec 9, 2023 • edited Loading

CyrusNajmabadi commented Dec 9, 2023

RikkiGibson commented Dec 12, 2023

This comment was marked as resolved.

cston commented Dec 15, 2023 • edited Loading

333fred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Dec 9, 2023 •

edited

Loading

cston commented Dec 15, 2023 •

edited

Loading