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

Prefer converting to ReadOnlySpan for spread optimization when we do not need to write #71733

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Prefer converting to ReadOnlySpan for spread optimization when we do not need to write #71733

merged 3 commits into from
Jan 25, 2024

Conversation

RikkiGibson
Copy link
Contributor

Closes #71720

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 19, 2024
RikkiGibson
RikkiGibson commented Jan 19, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
if (tryGetToArrayMethod(spanType, WellKnownType.System_Span_T, WellKnownMember.System_Span_T__ToArray, out var toArrayMethod)
|| tryGetToArrayMethod(spanType, WellKnownType.System_ReadOnlySpan_T, WellKnownMember.System_ReadOnlySpan_T__ToArray, out toArrayMethod))
if (tryGetToArrayMethod(spanType, WellKnownType.System_ReadOnlySpan_T, WellKnownMember.System_ReadOnlySpan_T__ToArray, out var toArrayMethod)
|| tryGetToArrayMethod(spanType, WellKnownType.System_Span_T, WellKnownMember.System_Span_T__ToArray, out toArrayMethod))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reordered these as I think the ReadOnlySpan case will now be more common

@RikkiGibson RikkiGibson marked this pull request as ready for review January 19, 2024 22:36
@RikkiGibson RikkiGibson requested a review from a team as a code owner January 19, 2024 22:36
@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler for review

@RikkiGibson RikkiGibson requested a review from a team January 22, 2024 23:34
@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1). Before I sign-off, I would like to see the requested test added. Also, please confirm that there are no other places where we request a Span from an array during collection expression lowering. The destination array that we create ourselves is an exception, of course.

@RikkiGibson
Copy link
Contributor Author

RikkiGibson commented Jan 24, 2024 •
edited
Loading

Also, please confirm that there are no other places where we request a Span from an array during collection expression lowering. The destination array that we create ourselves is an exception, of course.

That's correct, the only time we are converting an array to Span for this optimization is for the destination array.

Thinking on it now, we could probably simplify here by having a one-off bit of codegen for converting the array to writable Span, adjusting 'TryGetSpanConversion' to remove the "writableOnly" parameter, and making it simply return the "best" Span type for use as a receiver in a call to either Span/ReadOnlySpan.CopyTo(Span).

I will experiment with that change briefly tomorrow, out of time for today.

AlekseyTs
AlekseyTs reviewed Jan 25, 2024
View reviewed changes
src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs
""";

var comp = CreateCompilation([source, s_collectionExtensions], targetFramework: TargetFramework.Net80, options: TestOptions.ReleaseExe);
comp.VerifyEmitDiagnostics();
Copy link
Contributor

Choose a reason for hiding this comment

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

VerifyEmitDiagnostics

Minor, non-blocking suggestion: Consider calling VerifyDiagnoctics on result of CompileAndVerify instead. This should eliminate a redundant emit and will make the test faster a bit.

AlekseyTs
AlekseyTs approved these changes Jan 25, 2024
View reviewed changes
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 2)

@RikkiGibson RikkiGibson merged commit a7115c7 into dotnet:main Jan 25, 2024
25 checks passed
@ghost ghost added this to the Next milestone Jan 25, 2024
@Cosifne Cosifne modified the milestones: Next, 17.10 P1 Jan 30, 2024
@Cosifne Cosifne mentioned this pull request Feb 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@cston cston cston approved these changes

Assignees
No one assigned
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.10 P1
Development

Successfully merging this pull request may close these issues.

Collection expression spreads on arrays can result in variance exceptions
4 participants
@RikkiGibson @AlekseyTs @cston @Cosifne