Use single argument overload for [start..] of string or (ReadOnly)Span

[tats-u]

Uses the single argument overloads of string.Substring and (ReadOnly)Span.Slice instead of 2 arguments ones for lowering [start..].
This change shortens ILs and x64 assemblies.

Fixes #47629

TODO:

• Add tests for Memory
• Add tests for corlib Span
• Add second argument to the method to check equality of Slice method
• Use FailsILVerify.WithILVerifyMessage
• (Don't emit extra stloc.$n$ and ldloc.$n$)

[tats-u]

rebased to e8bae2f, the latest merge commit where the CI succeeds.
This is due to the CI failure due to the place not changed by me.

[333fred]

All perf changes need to be accompanied by benchmarks showing their impact, just "produces smaller il" is not enough. The linked issue has benchmarks from 4 years ago; that's both very out of date, and the benchmarking code has a lot of unrelated operations in it.

[tats-u]

https://github.com/tats-u/SubstringSliceBench

For ReadOnlySpan.Slice, the single argument overload is 1.7x faster.

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.3880/23H2/2023Update/SunValley3)
Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores
.NET SDK 9.0.100-preview.6.24328.19
  [Host]  : .NET 8.0.7 (8.0.724.31311), X64 RyuJIT AVX2
  LongRun : .NET 9.0.0 (9.0.24.32707), X64 RyuJIT AVX2

Job=LongRun  Runtime=.NET 9.0  Toolchain=net9.0  
IterationCount=100  LaunchCount=3  WarmupCount=15  

Method	Mean	Error	StdDev	Median	Ratio	MannWhitney(10%)	RatioSD	Allocated	Alloc Ratio
.Slice(int,int)	3.281 ns	0.0334 ns	0.1635 ns	3.182 ns	1.00	Base	0.00	-	NA
.Slice(int)	1.945 ns	0.0105 ns	0.0538 ns	1.930 ns	0.59	Faster	0.03	-	NA

For string.Substring, the difference is relatively small because InternalSubstring takes relatively long time.

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.3880/23H2/2023Update/SunValley3)
Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores
.NET SDK 9.0.100-preview.6.24328.19
  [Host]  : .NET 8.0.7 (8.0.724.31311), X64 RyuJIT AVX2
  LongRun : .NET 9.0.0 (9.0.24.32707), X64 RyuJIT AVX2

Job=LongRun  Runtime=.NET 9.0  Toolchain=net9.0  
IterationCount=100  LaunchCount=3  WarmupCount=15  

Method	Mean	Error	StdDev	Median	Ratio	MannWhitney(10%)	RatioSD	Gen0	Allocated	Alloc Ratio
.Substring(int,int)	28.20 ns	0.149 ns	0.762 ns	27.95 ns	1.00	Base	0.00	0.0140	88 B	1.00
.Substring(int)	27.29 ns	0.151 ns	0.786 ns	27.20 ns	0.97	Same	0.04	0.0140	88 B	1.00

[tats-u]

I'll have to compare the internal code of Substring's without the execution parts of InternalSubstring.

[333fred]

@stephentoub, a penny for your thoughts on this optimization?

[stephentoub]

@stephentoub, a penny for your thoughts on this optimization?

I've not looked at the PR, but from my perspective addressing it would be nice to have:
#47629 (comment)
It's not a huge deal, but we do shy away from using the range syntax in runtime due to this difference. It'd be nice if there wasn't any difference at all so folks didn't need to consider whether it might be an issue.

[tats-u]

we do shy away from using the range syntax in runtime due to this difference.

I agree with you.
This is why IDE0057 for Slice/Substring(int) sounds like the devil whispering for me and I've avoided to use [..start].
It's much more harmful than IDE0081, which sometimes may be treated so by some (legacy) developers. (I agree with IDE0081 for my opinion)

[tats-u]

Should I optimize for (ReadOnly)Memory or stringVariable.AsSpan()[start..] (and stringVariable.AsSpan()[start..end])?
Should we suggest users to rewrite .AsSpan()[start..end] to .AsSpan(start, length) in Analyzer?
Also .AsSpan(start..end) can be rewritten to .AsSpan(start, length) or .AsSpan(start), which has less overhead.

[CyrusNajmabadi]

I'ma fan of this sort of optimization. It means that people can use the more modern/idiomatic patterns, without worry about a perf cliff.

[tats-u]

If the annoying src\Workspaces\Core\Portable\Workspace\Solution\Solution.cs(570,37): error CS0103: (NETCORE_ENGINEERING_TELEMETRY=Build) The name '_compilationState' does not exist in the current context still occurred, I wonder how I would deal with it.
I believe it's not my fault.

Update: eventually didn't occur

[tats-u]

roslyn/src/Compilers/CSharp/Test/Semantic/Semantics/SpanStackSafetyTests.cs

Lines 1725 to 1727 in cad8698

	// (15,9): error CS0029: Cannot implicitly convert type 'System.Span<int>' to 'System.ValueType'
	// default(Span<int>).ToString();
	Diagnostic(ErrorCode.ERR_NoImplicitConv, "default(Span<int>)").WithArguments("System.Span<int>", "System.ValueType").WithLocation(15, 9)

In the current change, this diagnostic doesn't seem to come up.

  Failed Microsoft.CodeAnalysis.CSharp.UnitTests.SpanStackSafetyTests.BaseMethods [92 ms]
  Error Message:
   
Expected:
                Diagnostic(ErrorCode.ERR_NoImplicitConv, "default(Span<int>)").WithArguments("System.Span<int>", "object").WithLocation(12, 9),
                Diagnostic(ErrorCode.ERR_NoImplicitConv, "default(Span<int>)").WithArguments("System.Span<int>", "System.ValueType").WithLocation(15, 9)
Actual:
                // (12,9): error CS0029: Cannot implicitly convert type 'System.Span<int>' to 'object'
                //         default(Span<int>).GetType();
                Diagnostic(ErrorCode.ERR_NoImplicitConv, "default(Span<int>)").WithArguments("System.Span<int>", "object").WithLocation(12, 9)
Diff:
-->                 Diagnostic(ErrorCode.ERR_NoImplicitConv, "default(Span<int>)").WithArguments("System.Span<int>", "System.ValueType").WithLocation(15, 9)
Expected: True
Actual:   False
  Stack Trace:
     at Microsoft.CodeAnalysis.DiagnosticExtensions.Verify(IEnumerable`1 actual, DiagnosticDescription[] expected, Boolean errorCodeOnly) in /_/src/Compilers/Test/Core/Diagnostics/DiagnosticExtensions.cs:line 99
   at Microsoft.CodeAnalysis.DiagnosticExtensions.Verify(IEnumerable`1 actual, DiagnosticDescription[] expected) in /_/src/Compilers/Test/Core/Diagnostics/DiagnosticExtensions.cs:line 48
   at Microsoft.CodeAnalysis.DiagnosticExtensions.Verify(ImmutableArray`1 actual, DiagnosticDescription[] expected) in /_/src/Compilers/Test/Core/Diagnostics/DiagnosticExtensions.cs:line 63
   at Microsoft.CodeAnalysis.DiagnosticExtensions.VerifyDiagnostics[TCompilation](TCompilation c, DiagnosticDescription[] expected) in /_/src/Compilers/Test/Core/Diagnostics/DiagnosticExtensions.cs:line 109
   at Microsoft.CodeAnalysis.CSharp.UnitTests.SpanStackSafetyTests.BaseMethods() in /_/src/Compilers/CSharp/Test/Semantic/Semantics/SpanStackSafetyTests.cs:line 1721

   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

[333fred]

Also, it looks like there are test failures.

[tats-u]

I knew that but the CI insists that I hadn't fixed the places where I fixed in e2b3f40.
I'll relaunch the CI to try to make sure to make it apply the change where it complained.

[tats-u]

I knew that but the CI insists that I hadn't fixed the places where I fixed in e2b3f40.

I saw the console output, but the ILs in Expected are those before that commit.

[tats-u]

All existing unit tests now pass, but I couldn't understand what the following in Rebuild tests means:

Specified argument was out of the range of valid values.
Parameter name: position