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

Optimize single spread of IEnumerable to array #75847

Merged
merged 10 commits into from
Nov 15, 2024

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Nov 10, 2024

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 calling System.Linq.Enumerable.ToArray<T>(IEnumerable<T>) in most cases when the spread value is convertible to IEnumerable.

@RikkiGibson RikkiGibson requested a review from a team as a code owner November 10, 2024 00:53
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 10, 2024
/// </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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's now 3 possible optimizations this method will try, in order:

  1. List.ToArray if the spread value is a list
  2. IEnumerable.ToArray if our checks say we should convert the spread value in this way
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider capturing this information in a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

int[] arr1 = [..arr, ..arr];
arr1.Report();
}
public static object[] M(string[] arr) => [..arr];
Copy link
Member

Choose a reason for hiding this comment

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

object[]

Let's also test when the target type is Span<object>.

Copy link
Member

Choose a reason for hiding this comment

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

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];

@RikkiGibson RikkiGibson requested a review from a team November 11, 2024 20:43
using System;

C.M1(["a"]);
C.M2(["b"]);
Copy link
Member

@cston cston Nov 11, 2024

Choose a reason for hiding this comment

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

C.M2(["b"]) creates an object[] rather than a string[]. Consider rewriting as:

string[] s = ["a"];
C.M1(s);
C.M2(s);

@RikkiGibson RikkiGibson requested a review from a team November 12, 2024 00:00
@RikkiGibson

This comment was marked as resolved.

This comment was marked as resolved.

@AlekseyTs
Copy link
Contributor

@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?

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 9)

@RikkiGibson
Copy link
Contributor Author

@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?

Yes, I think #75863 represents the extent of the further improvement we could realistically make to the general case of #71755.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 10)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
4 participants