-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Support using string.Concat(ReadOnlySpan<string>) for string lowering
#81788
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
base: main
Are you sure you want to change the base?
Conversation
Closes dotnet#74538. We now support using `string.Concat(ReadOnlySpan<string>)`, instead of `string.Concat(string[])`, where available. This allows us to avoid an allocation of an array, and takes advantage of the inline array support already added for collection expressions and `params ReadOnlySpan<T>`. On runtimes that do not support inline arrays, the new overloads do not light up. Also added is support for interpolated strings to lower to this form, preferring that over `DefaultInterpolatedStringHandler` when all components are strings for performance.
src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
Outdated
Show resolved
Hide resolved
|
@dotnet/roslyn-compiler for reviews. /cc @stephentoub |
|
@333fred It looks like there are legitimate test failures |
src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs
Outdated
Show resolved
Hide resolved
It would be good to add a comment that this method mutates Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs:72 in 3eb40ea. [](commit_id = 3eb40ea, deletion_comment = False) |
It even frees the builder In reply to: 3693188522 Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs:72 in 3eb40ea. [](commit_id = 3eb40ea, deletion_comment = False) |
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs
Outdated
Show resolved
Hide resolved
| if (_compilation.Assembly.RuntimeSupportsInlineArrayTypes | ||
| && TryGetSpecialTypeMethod(originalSyntax, SpecialMember.System_String__ConcatReadOnlySpanString, out concatMethod, isOptional: true)) | ||
| { | ||
| finalArguments = [CreateAndPopulateSpanFromInlineArray( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .maxstack 4 | ||
| .locals init (char V_0, //c | ||
| char V_1, //d | ||
| System.Runtime.CompilerServices.InlineArray6<string> V_2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| #nullable disable | ||
|
|
||
| using System.Linq; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are both added usings used?
src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenSpanBasedStringConcatTests.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenSpanBasedStringConcatTests.cs
Show resolved
Hide resolved
| comp.MakeMemberMissing((SpecialMember)missingUnimportantMember.Value); | ||
| } | ||
|
|
||
| verifier = CompileAndVerify(comp, expectedOutput: ExecutionConditionUtil.IsCoreClr ? "abcde" : null, verify: Verification.Fails); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar for added tests below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these all Fail regardless, as InlineArrayAsReadOnlySpan does not pass ILVerify.
| IL_0040: ldloca.s V_1 | ||
| IL_0042: ldc.i4.5 | ||
| IL_0043: call "System.ReadOnlySpan<string> <PrivateImplementationDetails>.InlineArrayAsReadOnlySpan<System.Runtime.CompilerServices.InlineArray5<string>, string>(in System.Runtime.CompilerServices.InlineArray5<string>, int)" | ||
| IL_0048: call "string string.Concat(params System.ReadOnlySpan<string>)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .locals init (string V_0, //s | ||
| System.Runtime.CompilerServices.InlineArray5<string> V_1, | ||
| System.Runtime.CompilerServices.InlineArray6<string> V_2, | ||
| System.Runtime.CompilerServices.InlineArray7<string> V_3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| System.Console.WriteLine(s + s + s + s + s); | ||
| """; | ||
|
|
||
| var comp = CreateCompilation(source, targetFramework: TargetFramework.Net100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm intentionally testing where string.Concat(params ReadOnlySpan<string>) exists, but the runtime does not have inline array support.
| var comp = CreateCompilation(source, targetFramework: TargetFramework.Net100); | ||
| var verifier = CompileAndVerify(comp, expectedOutput: ExecutionConditionUtil.IsCoreClr ? "aaaaa" : null, verify: Verification.FailsPEVerify); | ||
| verifier.VerifyDiagnostics(); | ||
| verifier.VerifyIL("<top-level-statements-entry-point>", """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #nullable disable | ||
|
|
||
| using System.Linq; | ||
| using Microsoft.CodeAnalysis.CSharp.Test.Utilities; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this using used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming you meant the Linq using, yes, Enumerable.Repeat is used below.
| 123 | ||
| 1234 | ||
| 12345 | ||
| """ : null, verify: Verification.Skipped); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| System.Console.WriteLine({expression}); | ||
| """; | ||
|
|
||
| var comp = CreateCompilation(code, targetFramework: TargetFramework.Net100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I'm intentionally testing when params ReadOnlySpan<string> exists, but inline arrays are not supported by the runtime.
| IL_0002: initobj "System.Runtime.CompilerServices.InlineArray5<string>" | ||
| IL_0008: ldloca.s V_0 | ||
| IL_000a: ldc.i4.0 | ||
| IL_000b: call "ref string <PrivateImplementationDetails>.InlineArrayElementRef<System.Runtime.CompilerServices.InlineArray5<string>, string>(ref System.Runtime.CompilerServices.InlineArray5<string>, int)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| IL_004d: stind.ref | ||
| IL_004e: ldloca.s V_0 | ||
| IL_0050: ldc.i4.5 | ||
| IL_0051: call "System.ReadOnlySpan<string> <PrivateImplementationDetails>.InlineArrayAsReadOnlySpan<System.Runtime.CompilerServices.InlineArray5<string>, string>(in System.Runtime.CompilerServices.InlineArray5<string>, int)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Are we testing scenarios when Span and/or ReadOnlySpan are missing? |
|
It looks like we are lacking coverage for a scenario when |
|
We need to decide whether we are comfortable with a possible stack bloating that the current implementation might lead to. Including whether we might want to have a threshold on the length of the inline array that we are willing to put on the stack. |
|
Done with review pass (commit 6) |
Based on some comments on dotnet#81788, I wanted to see what an implementation of sharing inline arrays across different calls might look like. This is very much still a prototype, and needs plenty of cleaning up (and extensive testing), but the concept is good enough to be reviewed, I think. The general strategy is that, for by-value and scoped `Span/ReadOnlySpan` parameters that are directly passed a collection expression (either explicitly as a collection expression or implicitly as a params parameter), we know the precise lifetime that span needs to remain valid: from the start of the argument position in code until the end of the method call. We can record this information during local rewriting to know, for every type, what the maximum stack space needed at any given time is (including what might be needed for multiple nested calls with the same element type occurring at the same time). We then substitute in a "replace me with a real inline array ref" node. We can then pre-allocate an inline array of that max size, and slice off bits for each call as necessary, replacing the placeholders with real slicing in a new pass I've called the `TransientInlineArrayRewriter`. Since these arrays might be used later in the method, we do have to clear the arrays after use if the element type could be a reference type so the GC can reclaim their values. As I mentioned, this is not yet ready for an in-depth code review. PROTOTYPE comments still exist, and there are several bugs failing tests that I haven't really looked into yet. But the general concept is ready for comment before I go further.
Closes #74538. We now support using
string.Concat(ReadOnlySpan<string>), instead ofstring.Concat(string[]), where available. This allows us to avoid an allocation of an array, and takes advantage of the inline array support already added for collection expressions andparams ReadOnlySpan<T>. On runtimes that do not support inline arrays, the new overloads do not light up. Also added is support for interpolated strings to lower to this form, preferring that overDefaultInterpolatedStringHandlerwhen all components are strings for performance.