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

[RikkiGibson]

Hi @tats-u, if you have some time, please try merging the latest main in, and we will try and help with investigating the CI failures.

[EgorBo]

Related: dotnet/runtime#122042 (while we want to improve the codegen in JIT, I think it'd be nice for Roslyn to emit span.Slice(X) instead of span.Slice(X, span.Length - X) from the codegen's perspective.

[kasperk81]

rebased and opened #81485

[tats-u]

I cannot spare time for this pull request for the time being, so I will close it for now. Is it better just to make it draft again?

@kasperk81 Merge instead of rebase is more recommended in this repository.

[kasperk81]

@kasperk81 Merge instead of rebase is more recommended in this repository.

isn't the rule don't rebase: when review has started?

anyways, your branch was 1700+ commits behind. i rebased to a single commit, cherry picked it on main and resolved conflicts

[tats-u]

Over a year ago, but I got reviews twice.
Ultimately, it is up to maintainers/reviewers.

[tats-u]

If you want to take over my work, I would like you to fix "(Don't emit extra stloc. n and ldloc. n )" in TODO. I remember it a little now.

[kasperk81]

Over a year ago, but I got reviews twice.

i was talking about the new pr. we can rebase and force push as we please before the maintainers have started the review.

If you want to take over my work, I would like you to fix

1. github pull request is a 'branch' under a 'namespace' in the target repo. the moment you open a pull reuqest, it becomes part of the repo. at this stage "take over my work" doesn't make sense.
2. your authorship is kept in the commit and when it'll be merged to main, you will still be the author of the commit. my co- contribution is of mechanical sort that i'm making sure if JIT devs are waiting for csc to emit single arg span overload for slice syntax, it gets done without further delays.
3. unless it's a regression i would like not to change functionality. i've checked and it's not a regression: stloc. n followed by ldloc. n is an existing pattern and not the kind which JIT is having problem optimizing.