Optimized string.Replace(char, char)

[gfoidl]

Description

The vectorized path uses Vector<ushort>, so on x64-cpu with AVX2 available (and enabled) the vectorized path processes 16 chars / ushorts at once.
For the remainder there are [0, Vector<ushort>.Count) elements left which are currently processed in a scalar way.
As the operation is idempotent, we can avoid this scalar processing by just processing the final Vector.Count elements vectorized again. We do this anyway to avoid an additional branch.

Also streamlined the beginning of the method by avoiding multiple return, thus collapsing the epilogs.

asm (excerpt) before

pushr15pushr14pushr12pushrdipushrsipushrbppushrbxsubrsp,20vzerouppermovrsi,rcxmovzxedi,dxmovzxebx,r8wcmpedi,ebxjne short M01_L00movrax,rsivzeroupperaddrsp,20poprbxpoprbppoprsipoprdipopr12popr14popr15retM01_L00:movr8,[rsi+8]learcx,[r8+0C]movr8d,[r8+8]movedx,edicall System.SpanHelpers.IndexOf(Char ByRef, Char, Int32)movebp,eaxtestebp,ebpjge short M01_L01movrax,rsivzeroupperaddrsp,20poprbxpoprbppoprsipoprdipopr12popr14popr15ret

asm (excerpt) after

pushr15pushr14pushr12pushrdipushrsipushrbppushrbxsubrsp,20vzerouppermovrsi,rcxmovzxedi,dxmovzxebx,r8wcmpedi,ebxje short M01_L00movr8,[rsi+8]learcx,[r8+0C]movr8d,[r8+8]movedx,edicall System.SpanHelpers.IndexOf(Char ByRef, Char, Int32)movebp,eaxtestebp,ebpjge short M01_L01 M01_L00:movrax,rsivzeroupperaddrsp,20poprbxpoprbppoprsipoprdipopr12popr14popr15ret

And instead of moving the source-ref, dest-ref forward, and decreasing the reaminingCount, direct indexing is used -- thus eliding two additions and making the loop bodies smaller.

asm (excerpt) before

M01_L03:vmovupdymm2,[rcx] vpcmpeqw ymm3,ymm2,ymm0vpandymm4,ymm1,ymm3vpandnymm2,ymm3,ymm2vporymm2,ymm4,ymm2vmovupd[rdx],ymm2addrcx,20addrdx,20addr14d,0FFFFFFF0cmpr14d,10jge short M01_L03

asm (excerpt) after

M01_L03:lear9,[rcx+rcx]vmovupdymm2,[rax+r9] vpcmpeqw ymm3,ymm2,ymm0vpandymm4,ymm1,ymm3vpandnymm2,ymm3,ymm2vporymm2,ymm4,ymm2vmovupd[rdx+r9],ymm2addrcx,10cmprcx,r8jle short M01_L03

Benchmark

info

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19043.1586 (21H1/May2021Update)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.100-preview.2.22153.17
[Host] : .NET 7.0.0 (7.0.22.15202), X64 RyuJIT
DefaultJob : .NET 7.0.0 (7.0.22.15202), X64 RyuJIT

| Method | Length | Mean | Error | StdDev | Ratio | RatioSD |
|-------- |------- |---------:|---------:|---------:|------:|--------:|
| Default | 16 | 19.26 ns | 0.443 ns | 0.435 ns | 1.00 | 0.00 |
| PR | 16 | 19.76 ns | 0.252 ns | 0.224 ns | 1.03 | 0.03 |
| | | | | | | |
| Default | 17 | 19.62 ns | 0.306 ns | 0.287 ns | 1.00 | 0.00 |
| PR | 17 | 19.77 ns | 0.161 ns | 0.134 ns | 1.01 | 0.02 |
| | | | | | | |
| Default | 24 | 26.78 ns | 0.326 ns | 0.289 ns | 1.00 | 0.00 |
| PR | 24 | 20.58 ns | 0.318 ns | 0.282 ns | 0.77 | 0.01 |
| | | | | | | |
| Default | 31 | 33.39 ns | 0.321 ns | 0.268 ns | 1.00 | 0.00 |
| PR | 31 | 21.32 ns | 0.346 ns | 0.307 ns | 0.64 | 0.01 |
| | | | | | | |
| Default | 32 | 21.74 ns | 0.305 ns | 0.271 ns | 1.00 | 0.00 |
| PR | 32 | 22.38 ns | 0.453 ns | 0.539 ns | 1.03 | 0.03 |

There is quite a huge improvement when some elements are remaining, but a litte regression if no or just a few elements are remaining.
But in absolute numbers that regression is tiny.

Open questions

Right now Vector<ushort> is used.
This means that on hardware where 256 bit SIMD is supported 16 elements are processed at once. On hardware where only 128 bit SIMD is supported, it's 8 elements.

There is the debate / consideration about Vector<T>, Vector128<T>, Vector256<T> as xplat-intrinsics around, so here arises the question should the code be update to use Vector128<T> or Vector256<T> instead of Vector<T>?
In my opinion Vector<T> is a good choice here and should be kept as

• it works xplat with a single code-path
• with Vector128 it's "only" 8 chars at once, when it could be 16 -- so there should be a extra path for Vector256, complicating the code
• I assume for string.Replace(char, char) a length of >= 16 is quite common, so it's best when AVX2 is available

I checked the codegen for Vector128 and Vector256, it emits the same / similar code (that's why #67039, #67038 got opened).
The only question is for the count of elements processed at once, and potential warm-up, etc. that comes with AVX.

[danmoseley]

Nice - could you maybe get measurements for longer strings? I would imagine they are fairly common, and this change looks even better there.

I see our perf coverage is missing longer string(s) and probably should have one.
https://github.com/dotnet/performance/blob/7d5a03bc880a27b19cb61449aaa269d1c8f4f8e5/src/benchmarks/micro/libraries/System.Runtime/Perf.String.cs#L157-L160

[ghost]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[gfoidl]

measurements for longer strings?

| Method | Length | Mean | Error | StdDev | Median | Ratio | RatioSD |
|-------- |------- |----------:|---------:|----------:|----------:|------:|--------:|
| Default | 16 | 22.82 ns | 0.571 ns | 1.676 ns | 22.41 ns | 1.00 | 0.00 |
| PR | 16 | 21.82 ns | 0.510 ns | 1.030 ns | 21.70 ns | 0.96 | 0.07 |
| | | | | | | | |
| Default | 17 | 21.68 ns | 0.500 ns | 0.632 ns | 21.76 ns | 1.00 | 0.00 |
| PR | 17 | 21.19 ns | 0.380 ns | 0.337 ns | 21.17 ns | 0.99 | 0.03 |
| | | | | | | | |
| Default | 24 | 28.24 ns | 0.317 ns | 0.281 ns | 28.21 ns | 1.00 | 0.00 |
| PR | 24 | 22.37 ns | 0.362 ns | 0.321 ns | 22.36 ns | 0.79 | 0.01 |
| | | | | | | | |
| Default | 31 | 37.02 ns | 0.734 ns | 0.686 ns | 36.93 ns | 1.00 | 0.00 |
| PR | 31 | 22.95 ns | 0.477 ns | 0.423 ns | 23.03 ns | 0.62 | 0.01 |
| | | | | | | | |
| Default | 32 | 22.63 ns | 0.506 ns | 0.497 ns | 22.44 ns | 1.00 | 0.00 |
| PR | 32 | 23.76 ns | 0.527 ns | 0.493 ns | 23.76 ns | 1.05 | 0.03 |
| | | | | | | | |
| Default | 100 | 37.25 ns | 0.736 ns | 0.614 ns | 37.27 ns | 1.00 | 0.00 |
| PR | 100 | 34.26 ns | 0.628 ns | 0.557 ns | 34.32 ns | 0.92 | 0.01 |
| | | | | | | | |
| Default | 500 | 104.41 ns | 2.088 ns | 4.881 ns | 102.73 ns | 1.00 | 0.00 |
| PR | 500 | 97.96 ns | 1.858 ns | 2.837 ns | 98.46 ns | 0.93 | 0.06 |
| | | | | | | | |
| Default | 1000 | 201.91 ns | 4.234 ns | 12.484 ns | 199.50 ns | 1.00 | 0.00 |
| PR | 1000 | 185.13 ns | 3.775 ns | 7.363 ns | 183.77 ns | 0.92 | 0.07 |

Looks better in general, now only one regression in the len 32 case, but tiny in absolute time.

I see our perf coverage is missing longer string(s) and probably should have one.

Will add longer strings there.

[EgorBo]

question should the code be update to use Vector128 or Vector256 instead of Vector?

If we remove Vector path we'll regress Mono that supports Vector128 only in LLVM mode

In my opinion Vector is a good choice here and should be kept as

As you noted - It doesn't handle small inputs well when AVX is available 😢 e.g. #64899 (comment)

[EgorBo]

isn't it against the guidelines to perform an assignment inside an if ?

[gfoidl]

I don't know, but it gives nice machine code here 😉
It's about collapsing the epilogs for the first checks (oldChar == newChar, and firstIndex < 0).
Maybe I think too complicated now, but the other option would be using goto for this.

[tannergooding]

At the very least it would be good to have a comment calling out the assignment why it is being done here.

Otherwise, at a glance it may look like a potential bug or comparison using ==

[tannergooding]

But, the below is also the more "natural" pattern and more readable:

if(oldChar==newChar){returnthis;}intfirstIndex=IndexOf(oldChar);if(firstIndex<0){returnthis;}

Ideally the JIT would handle such a pattern "correctly" and optimize it down accordingly.

[gfoidl]

@tannergooding so what's my action here?

• add the comment explaining that the epilogs get collapsed in generated machine-code
• write it more naturally and file an issue for the JIT
• do both, and write it naturally once the JIT issue is fixed

(I'm leaning towards the last option, for perf-reasons -- except JIT issue will be fixed for .NET 7 😉)

[tannergooding]

I think writing it naturally and filing an issue for the JIT is the best choice and don't expect the cost to be significant here.

If the cost is more significant, then adding a comment calling out the assignment and why as well as filing an issue for the JIT is the next best option.

If the issue is actually being fixed for .NET 7, that's all the more reason to do the first approach.

[gfoidl]

Is the codegen issue tracked by #8883?

[gfoidl]

Code change here (back to where it was) with 30889ac
If I read the issue from the previous comment correct, so this should cover that case.

[EgorBo]

off-topic: gosh, the same line with raw pointers is basically

ushort*pDst=((ushort*)result._firstChar)[copyLength]

"Safe" Unsafe is killing me 🤦‍♂️

[gfoidl]

Same here 🙈 It reads (and writes) like a mess.
Pinning wasn't used here, so I didn't use it too -- also I expect a little bit of regression then.

[tannergooding]

Might become slightly more readable if the Unsafe.As was the outermost.

We could also expose an internal only Vector.LoadUnsafe(ref T, nuint index) API which would also simplify things here.

[EgorBo]

Perhaps worth adding an assert that current Debug.Assert(this.Length - i <= Vector<ushort>.Count) to make sure we won't skip any data?

[gfoidl]

Hm, I think in this case a test should fail?
I'll re-check the tests and make sure that case is covered.

[gfoidl]

Tests cover these cases, so I don't see a need for the Debug.Assert -- but I'll add it of course if you want.

runtime/src/libraries/Common/tests/Tests/System/StringTests.cs

Lines 4566 to 4578 in 8ed8517

	// -------------------- For Vector<ushort>.Count == 8 (SSE2 / ARM NEON) --------------------
	[InlineData("Aaaaaaaa",'A','a',"aaaaaaaa")]// Single iteration of vectorised path; no remainders through non-vectorised path
	// Three leading 'a's before a match (copyLength > 0), Single iteration of vectorised path; no remainders through non-vectorised path
	[InlineData("aaaAaaaaaaa",'A','a',"aaaaaaaaaaa")]
	// Single iteration of vectorised path; 3 remainders through non-vectorised path
	[InlineData("AaaaaaaaaAa",'A','a',"aaaaaaaaaaa")]
	// ------------------------- For Vector<ushort>.Count == 16 (AVX2) -------------------------
	[InlineData("AaaaaaaaAaaaaaaa",'A','a',"aaaaaaaaaaaaaaaa")]// Single iteration of vectorised path; no remainders through non-vectorised path
	// Three leading 'a's before a match (copyLength > 0), Single iteration of vectorised path; no remainders through non-vectorised path
	[InlineData("aaaAaaaaaaaAaaaaaaa",'A','a',"aaaaaaaaaaaaaaaaaaa")]
	// Single iteration of vectorised path; 3 remainders through non-vectorised path
	[InlineData("AaaaaaaaAaaaaaaaaAa",'A','a',"aaaaaaaaaaaaaaaaaaa")]
	// ----------------------------------- General test data -----------------------------------

[EgorBo]

LGTM, Thanks! cc @stephentoub@tannergooding

[tannergooding]

If we remove Vector path we'll regress Mono that supports Vector128 only in LLVM mode

@fanyang-mono is the work being done in Mono only covering Vector64/128 for ARM64 and only in LLVM or does it also extend to Mono JIT and x86/x64?

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

The vectorized path uses Vector<ushort>, so on x64-cpu with AVX2 available (and enabled) the vectorized path processes 16 chars / ushorts at once.
For the remainder there are [0, Vector<ushort>.Count) elements left which are currently processed in a scalar way.
As the operation is idempotent, we can avoid this scalar processing by just processing the final Vector.Count elements vectorized again. We do this anyway to avoid an additional branch.

Also streamlined the beginning of the method by avoiding multiple return, thus collapsing the epilogs.

asm (excerpt) before

pushr15pushr14pushr12pushrdipushrsipushrbppushrbxsubrsp,20vzerouppermovrsi,rcxmovzxedi,dxmovzxebx,r8wcmpedi,ebxjne short M01_L00movrax,rsivzeroupperaddrsp,20poprbxpoprbppoprsipoprdipopr12popr14popr15retM01_L00:movr8,[rsi+8]learcx,[r8+0C]movr8d,[r8+8]movedx,edicall System.SpanHelpers.IndexOf(Char ByRef, Char, Int32)movebp,eaxtestebp,ebpjge short M01_L01movrax,rsivzeroupperaddrsp,20poprbxpoprbppoprsipoprdipopr12popr14popr15ret

asm (excerpt) after

pushr15pushr14pushr12pushrdipushrsipushrbppushrbxsubrsp,20vzerouppermovrsi,rcxmovzxedi,dxmovzxebx,r8wcmpedi,ebxje short M01_L00movr8,[rsi+8]learcx,[r8+0C]movr8d,[r8+8]movedx,edicall System.SpanHelpers.IndexOf(Char ByRef, Char, Int32)movebp,eaxtestebp,ebpjge short M01_L01 M01_L00:movrax,rsivzeroupperaddrsp,20poprbxpoprbppoprsipoprdipopr12popr14popr15ret

And instead of moving the source-ref, dest-ref forward, and decreasing the reaminingCount, direct indexing is used -- thus eliding two additions and making the loop bodies smaller.

asm (excerpt) before

M01_L03:vmovupdymm2,[rcx] vpcmpeqw ymm3,ymm2,ymm0vpandymm4,ymm1,ymm3vpandnymm2,ymm3,ymm2vporymm2,ymm4,ymm2vmovupd[rdx],ymm2addrcx,20addrdx,20addr14d,0FFFFFFF0cmpr14d,10jge short M01_L03

asm (excerpt) after

M01_L03:lear9,[rcx+rcx]vmovupdymm2,[rax+r9] vpcmpeqw ymm3,ymm2,ymm0vpandymm4,ymm1,ymm3vpandnymm2,ymm3,ymm2vporymm2,ymm4,ymm2vmovupd[rdx+r9],ymm2addrcx,10cmprcx,r8jle short M01_L03

Benchmark

info

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19043.1586 (21H1/May2021Update)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.100-preview.2.22153.17
[Host] : .NET 7.0.0 (7.0.22.15202), X64 RyuJIT
DefaultJob : .NET 7.0.0 (7.0.22.15202), X64 RyuJIT

| Method | Length | Mean | Error | StdDev | Ratio | RatioSD |
|-------- |------- |---------:|---------:|---------:|------:|--------:|
| Default | 16 | 19.26 ns | 0.443 ns | 0.435 ns | 1.00 | 0.00 |
| PR | 16 | 19.76 ns | 0.252 ns | 0.224 ns | 1.03 | 0.03 |
| | | | | | | |
| Default | 17 | 19.62 ns | 0.306 ns | 0.287 ns | 1.00 | 0.00 |
| PR | 17 | 19.77 ns | 0.161 ns | 0.134 ns | 1.01 | 0.02 |
| | | | | | | |
| Default | 24 | 26.78 ns | 0.326 ns | 0.289 ns | 1.00 | 0.00 |
| PR | 24 | 20.58 ns | 0.318 ns | 0.282 ns | 0.77 | 0.01 |
| | | | | | | |
| Default | 31 | 33.39 ns | 0.321 ns | 0.268 ns | 1.00 | 0.00 |
| PR | 31 | 21.32 ns | 0.346 ns | 0.307 ns | 0.64 | 0.01 |
| | | | | | | |
| Default | 32 | 21.74 ns | 0.305 ns | 0.271 ns | 1.00 | 0.00 |
| PR | 32 | 22.38 ns | 0.453 ns | 0.539 ns | 1.03 | 0.03 |

There is quite a huge improvement when some elements are remaining, but a litte regression if no or just a few elements are remaining.
But in absolute numbers that regression is tiny.

Open questions

Right now Vector<ushort> is used.
This means that on hardware where 256 bit SIMD is supported 16 elements are processed at once. On hardware where only 128 bit SIMD is supported, it's 8 elements.

There is the debate / consideration about Vector<T>, Vector128<T>, Vector256<T> as xplat-intrinsics around, so here arises the question should the code be update to use Vector128<T> or Vector256<T> instead of Vector<T>?
In my opinion Vector<T> is a good choice here and should be kept as

• it works xplat with a single code-path
• with Vector128 it's "only" 8 chars at once, when it could be 16 -- so there should be a extra path for Vector256, complicating the code
• I assume for string.Replace(char, char) a length of >= 16 is quite common, so it's best when AVX2 is available

I checked the codegen for Vector128 and Vector256, it emits the same / similar code (that's why #67039, #67038 got opened).
The only question is for the count of elements processed at once, and potential warm-up, etc. that comes with AVX.

Author:	gfoidl
Assignees:	-
Labels:	area-System.Runtime, community-contribution
Milestone:	-

[fanyang-mono]

If we remove Vector path we'll regress Mono that supports Vector128 only in LLVM mode

@fanyang-mono is the work being done in Mono only covering Vector64/128 for ARM64 and only in LLVM or does it also extend to Mono JIT and x86/x64?

On arm64 SIMD is only supported when using the LLVM backend. On x86/x64, SIMD is supported either when LLVM is on or when LLVM is off and using Vector128. The code lives here:
https://github.com/dotnet/runtime/blob/main/src/mono/mono/mini/mini.h#L316-L326

[EgorBo]

On arm64 SIMD is only supported when using the LLVM backend. On x86/x64, SIMD is supported either when LLVM is on or when LLVM is off and using Vector128. The code lives here: https://github.com/dotnet/runtime/blob/main/src/mono/mono/mini/mini.h#L316-L326

The main question basically is - can we replace Vector<T> implementation with Vector128?
So from my understanding on the main mono targets (mobiles) Vector<T> is not supported without LLVM so I assume we can replace it since with LLVM both Vector<T> and Vector128 are supported everywhere?

[gfoidl]

Besides that do we have any data about typical ranges of string length where chars should be replaced? (from api metrics, etc)
With Vector128 it's only 8 chars at once. Will be good for short strings, but I expect it will regress on longer strings as Vector256 processes 16 chars at once.

[EgorBo]

btw, I've just checked - Mono on x64 (Linux) doesn't support Vector128 without LLVM. It prints false for all Sse*.IsSupported

while Vector<T> works just fine

[tannergooding]

With Vector128 it's only 8 chars at once. Will be good for short strings, but I expect it will regress on longer strings as Vector256 processes 16 chars at once.

It's going to depend on hardware, among other things.

Typically, well-optimized C/C++ code defines the cutoff as around 128 to 256-bytes (2-4 cache lines) and will optionally use V256 in the cases where the data is bigger than this. A lot of our C# code defines a cutoff of 64-bytes instead (1 cache line).

[tannergooding]

-- Where I'm defining "well-optimized" as some of the core memory and string functions provided by the C Runtime for MSVC, GCC, and Clang as they are some of the most critical, most profiled, and most deeply invested in pieces of code that are vectorized across the board.

[fanyang-mono]

btw, I've just checked - Mono on x64 (Linux) doesn't support Vector128 without LLVM. It prints false for all Sse*.IsSupported

while Vector<T> works just fine

This is different than what I posted earlier. Let me double check.

[fanyang-mono]

Mono on x64 (Linux) doesn't support Vector128 without LLVM, while Vector works just fine.

It is a little bit confusing to say that if Vector or Vector128 is supported on a specific platform. Because mono is still in the process of adding SIMD intrinsics for the methods. Recently, we just added some for arm64 and some for amd64.

Regarding to what is concerned to this PR, these are the methods that we would like to know if they are intrinsified/supported:

Vector.IsHardwareAccelerated
Vector<ushort>.Count
Vector.Equals
Vector.ConditionalSelect

And

Vector128.IsHardwareAccelerated
Vector128<ushort>.Count
Vector128.Equals
Vector128.ConditionalSelect

Currently, on amd64 some SIMD instructions were added for Vector128 but there isn't any methods being intrinsified without LLVM. With LLVM, Vector128.Equals and Vector128<ushort>.Count are currently supported to emit SIMD intrinsics. Support of Vector128.IsHardwareAccelerated could be easily added as well.

On amd64, Vector.Equals and Vector.ConditionalSelect are not intrinsified yet, but the other two are supported to emit SIMD instructions. This is true for both with and without LLVM.

In summary, currently, neither Vector128 nor Vector is be fully accelerated for Mono, in terms of this PR. We should choose based on theoretical excellence. The full SIMD intrinsics support will be there one day, hopefully soon.

[danmoseley]

Thanks @gfoidl

[danmoseley]

After discussion with @stephentoub , I'm going to backport this to .NET 7. While not meeting the letter of the criteria, it really should have gone in yesterday, before the snap, but CI was broken; and this was open for months, a with fair bit of waiting on us.

[danmoseley]

/backport to release/7.0-rc1

[github-actions]

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2872287360

[stephentoub]

@gfoidl, at least on my machine, comparing string.Replace in .NET 6 vs .NET 7, multiple examples I've tried have shown .NET 7 to have regressed, e.g.

conststringInput=""" Whose woods these are I think I know. His house is in the village though; He will not see me stopping here To watch his woods fill up with snow. My little horse must think it queer To stop without a farmhouse near Between the woods and frozen lake The darkest evening of the year. He gives his harness bells a shake To ask if there is some mistake. The only other sound’s the sweep Of easy wind and downy flake. The woods are lovely, dark and deep, But I have promises to keep, And miles to go before I sleep, And miles to go before I sleep. """;[Benchmark]publicstringReplace()=>Input.Replace('I','U');

Method	Runtime	Mean	Ratio
Replace	.NET 6.0	108.1 ns	1.00
Replace	.NET 7.0	136.0 ns	1.26

Do you see otherwise?

[gfoidl]

Hm, that is not expected...

When i duplicate the string.Replace(char, char)-method in order to compare the old and the new implementation both on .NET 7 then I see

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19043.1889 (21H1/May2021Update)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.100-preview.7.22377.5
[Host] : .NET 7.0.0 (7.0.22.37506), X64 RyuJIT
DefaultJob : .NET 7.0.0 (7.0.22.37506), X64 RyuJIT

Method	Mean	Error	StdDev	Median	Ratio	RatioSD
Default	142.0 ns	3.48 ns	9.98 ns	138.6 ns	1.00	0.00
PR	132.9 ns	2.68 ns	3.40 ns	132.8 ns	0.92	0.07

so a result I'd expect, as after the vectorized loop 6 chars are remaining that the old-code processes in the for-loop whilst the new-code does one vectorized pass.

I checked the dasm (via DisassemblyDiagnoser of BDN) and that looks OK.

Can this be something from different machine-code layout (loops), PGO, etc. that causes the difference between .NET 6 and .NET 7?
How can I investigate this further -- need some guidance on how to check code-layout please.

[stephentoub]

Thanks, @gfoidl. Do you see a similar 6 vs 7 difference as I do? (It might not be specific to this PR.) @EgorBo, can you advise?

[tannergooding]

When i duplicate the string.Replace(char, char)-method in order to compare the old and the new implementation both on .NET 7 then I see

This could be related to stale PGO data

[danmoseley]

Is there POGO data en-route that has trained with this change in place? I am not sure how to follow it.

[danmoseley]

Also, it wouldn't matter here, but are we consuming POGO data trained on main bits in the release branches?

[stephentoub]

I don't think this particular case is related to stale PGO data. I set COMPlus_JitDisablePGO=1, and I still see an ~20% regression from .NET 6 to .NET 7.

[danmoseley]

I ran the example above with

varconfig=DefaultConfig.Instance.AddJob(Job.Default.WithRuntime(CoreRuntime.Core31).WithEnvironmentVariable("COMPlus_JitDisablePGO","1")).AddJob(Job.Default.WithRuntime(CoreRuntime.Core60).WithEnvironmentVariable("COMPlus_JitDisablePGO","1")).AddJob(Job.Default.WithRuntime(CoreRuntime.CreateForNewVersion("net7.0",".NET 7.0")).WithEnvironmentVariable("COMPlus_JitDisablePGO","1")).AddJob(Job.Default.WithRuntime(ClrRuntime.Net48).WithEnvironmentVariable("COMPlus_JitDisablePGO","1")).AddJob(Job.Default.WithRuntime(CoreRuntime.Core31).WithEnvironmentVariable("COMPlus_JitDisablePGO","0")).AddJob(Job.Default.WithRuntime(CoreRuntime.Core60).WithEnvironmentVariable("COMPlus_JitDisablePGO","0")).AddJob(Job.Default.WithRuntime(CoreRuntime.CreateForNewVersion("net7.0",".NET 7.0")).WithEnvironmentVariable("COMPlus_JitDisablePGO","0").AsBaseline()).AddJob(Job.Default.WithRuntime(ClrRuntime.Net48).WithEnvironmentVariable("COMPlus_JitDisablePGO","0"));BenchmarkRunner.Run(typeof(Program).Assembly,args:args,config:config);

and got

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22000.856/21H2)
Intel Core i7-10510U CPU 1.80GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.100-rc.2.22426.5
[Host] : .NET 7.0.0 (7.0.22.42212), X64 RyuJIT AVX2
Job-DGTURM : .NET 6.0.8 (6.0.822.36306), X64 RyuJIT AVX2
Job-PYGDYG : .NET 7.0.0 (7.0.22.42212), X64 RyuJIT AVX2
Job-ZEPFOF : .NET Core 3.1.28 (CoreCLR 4.700.22.36202, CoreFX 4.700.22.36301), X64 RyuJIT AVX2
Job-PSEWWK : .NET Framework 4.8 (4.8.4510.0), X64 RyuJIT VectorSize=256
Job-WGVIGL : .NET 6.0.8 (6.0.822.36306), X64 RyuJIT AVX2
Job-HBSVYM : .NET 7.0.0 (7.0.22.42212), X64 RyuJIT AVX2
Job-VWWZUC : .NET Core 3.1.28 (CoreCLR 4.700.22.36202, CoreFX 4.700.22.36301), X64 RyuJIT AVX2
Job-LDCOEC : .NET Framework 4.8 (4.8.4510.0), X64 RyuJIT VectorSize=256
| Method | EnvironmentVariables | Runtime | Mean | Error | StdDev | Median | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio |
|-------- |------------------------ |------------------- |---------:|---------:|---------:|---------:|------:|--------:|-------:|----------:|------------:|
| Replace | COMPlus_JitDisablePGO=0 | .NET 6.0 | 130.5 ns | 6.76 ns | 18.51 ns | 124.0 ns | 0.92 | 0.17 | 0.3269 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=0 | .NET 7.0 | 144.0 ns | 2.95 ns | 5.54 ns | 142.5 ns | 1.00 | 0.00 | 0.3271 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=0 | .NET Core 3.1 | 822.1 ns | 16.09 ns | 23.07 ns | 814.0 ns | 5.69 | 0.31 | 0.3262 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=0 | .NET Framework 4.8 | 750.2 ns | 28.86 ns | 82.82 ns | 730.3 ns | 4.97 | 0.49 | 0.3262 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1 | .NET 6.0 | 127.1 ns | 2.64 ns | 4.75 ns | 126.4 ns | 0.88 | 0.05 | 0.3269 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1 | .NET 7.0 | 144.5 ns | 2.96 ns | 5.97 ns | 144.1 ns | 1.01 | 0.06 | 0.3271 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1 | .NET Core 3.1 | 936.2 ns | 17.96 ns | 22.06 ns | 931.9 ns | 6.50 | 0.37 | 0.3262 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1 | .NET Framework 4.8 | 673.2 ns | 12.41 ns | 23.91 ns | 670.5 ns | 4.68 | 0.23 | 0.3262 | 1.34 KB | 1.00 |

code https://gist.github.com/danmoseley/c31bc023d6ec671efebff7352e3b3251

(should we be surprised that disabling PGO didn't make any difference? perhaps it doesn't exercise this method? cc @AndyAyersMS )

[danmoseley]

and just for interest

| Method | EnvironmentVariables | Runtime | Mean | Error | StdDev | Median | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio |
|-------- |---------------------------------------------------- |--------- |-----------:|---------:|----------:|-----------:|------:|--------:|-------:|----------:|------------:|
| Replace | COMPlus_JitDisablePGO=1 | .NET 6.0 | 127.8 ns | 2.55 ns | 5.91 ns | 125.8 ns | 0.95 | 0.05 | 0.3266 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1 | .NET 7.0 | 141.0 ns | 2.73 ns | 2.42 ns | 141.1 ns | 1.00 | 0.00 | 0.3271 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1,COMPlus_EnableAVX2=0 | .NET 6.0 | 163.9 ns | 3.35 ns | 4.81 ns | 163.8 ns | 1.15 | 0.05 | 0.3269 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1,COMPlus_EnableAVX2=0 | .NET 7.0 | 184.9 ns | 3.59 ns | 4.79 ns | 183.7 ns | 1.32 | 0.05 | 0.3271 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1,COMPlus_EnableAVX=0 | .NET 6.0 | 176.1 ns | 3.44 ns | 4.09 ns | 175.9 ns | 1.25 | 0.03 | 0.3269 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1,COMPlus_EnableAVX=0 | .NET 7.0 | 192.1 ns | 3.81 ns | 4.53 ns | 190.1 ns | 1.37 | 0.05 | 0.3271 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1,COMPlus_EnableHWIntrinsic=0 | .NET 6.0 | 1,057.4 ns | 20.95 ns | 40.86 ns | 1,047.2 ns | 7.65 | 0.35 | 0.3262 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1,COMPlus_EnableHWIntrinsic=0 | .NET 7.0 | 947.1 ns | 13.34 ns | 11.83 ns | 948.3 ns | 6.72 | 0.15 | 0.3262 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1,COMPlus_EnableSSE3=0 | .NET 6.0 | 496.0 ns | 51.61 ns | 152.17 ns | 463.3 ns | 3.67 | 1.67 | 0.3269 | 1.34 KB | 1.00 |
| Replace | COMPlus_JitDisablePGO=1,COMPlus_EnableSSE3=0 | .NET 7.0 | 395.3 ns | 14.32 ns | 41.10 ns | 388.4 ns | 2.95 | 0.27 | 0.3271 | 1.34 KB | 1.00 |

[gfoidl]

Do you see a similar 6 vs 7 difference as I do?

Yes (sorry for slow response, was Sunday...).
@danmoseley thanks for your numbers.

This is the machine code I get (from BDN) when run @danmoseley's benchmark (.NET 7 only). Left there some comments.

; Program.Replace()movrcx,1C003C090A0movrcx,[rcx]movedx,49movr8d,55jmp qword ptr [7FFEFA7430C0]; Total bytes of code 30; System.String.Replace(Char, Char)pushr15pushr14pushrdipushrsipushrbppushrbxsubrsp,28vzerouppermovrsi,rcxmovedi,edxmovebx,r8dmovzxecx,dimovzxr8d,bxcmpecx,r8dje near ptr M01_L09learcx,[rsi+0C]movr8d,[rsi+8]movsxrdx,dicall qword ptr [7FFEFA7433C0]movebp,eaxtestebp,ebpjge short M01_L00movrax,rsi ; uncommon case, could jump to M01_L09 insteadvzeroupperaddrsp,28poprbxpoprbppoprsipoprdipopr14popr15retM01_L00:movecx,[rsi+8]subecx,ebpmovr14d,ecxmovecx,[rsi+8]call System.String.FastAllocateString(Int32)movr15,raxtestebp,ebpjg near ptr M01_L10 ; should be common path, I don't expect to jump to the end, then back to hereM01_L01:moveax,ebplearax,[rsi+rax*2+0C]cmp[r15],r15bmovedx,ebpleardx,[r15+rdx*2+0C]xorecx,ecxcmp dword ptr [rsi+8],10jl near ptr M01_L07movzxr8d,diimulr8d,10001 ; this is tracked in https://github.com/dotnet/runtime/issues/67038, .NET 6 has the same issue, so no difference expected vmovd xmm0,r8dvpbroadcastdymm0,xmm0 ; should be vpbroadcastb, see comment abovemovzxr8d,bximulr8d,10001 vmovd xmm1,r8dvpbroadcastdymm1,xmm1 ; vpbroadcastb (see above)cmpr14,10jbe short M01_L03addr14,0FFFFFFFFFFFFFFF0M01_L02:lear8,[rax+rcx*2]vmovupdymm2,[r8] vpcmpeqw ymm3,ymm2,ymm0vpandymm4,ymm3,ymm1 ; the vpand, vpandn, vpor series should be vpblendvb, https://github.com/dotnet/runtime/issues/67039 tracked thisvpandnymm2,ymm3,ymm2 ; the "duplicated code for string.Replace" method emits vpblendvb as expected, butvporymm2,ymm4,ymm2 ; if string.Replace from .NET 7.0.0 (7.0.22.42212) (.NET SDK=7.0.100-rc.2.22426.5) is used, then it's this serieslear8,[rdx+rcx*2]vmovupd[r8],ymm2addrcx,10cmprcx,r14jb short M01_L02M01_L03:movecx,[rsi+8]addecx,0FFFFFFF0addrsi,0Clearsi,[rsi+rcx*2]vmovupdymm2,[rsi] vpcmpeqw ymm3,ymm2,ymm0vpandymm0,ymm3,ymm1vpandnymm1,ymm3,ymm2vporymm2,ymm0,ymm1learax,[r15+0C]learax,[rax+rcx*2]vmovupd[rax],ymm2jmp short M01_L08M01_L04:movzxr8d,word ptr [rax+rcx*2]lear9,[rdx+rcx*2]movzxr10d,dicmpr8d,r10dje short M01_L05 ; not relevant for .NET 6 -> .NET 7 comparison in this test-case, butjmp short M01_L06 ; one jump could be avoided?!M01_L05:movzxr8d,bxM01_L06:mov[r9],r8wincrcxM01_L07:cmprcx,r14jb short M01_L04M01_L08:movrax,r15vzeroupperaddrsp,28poprbxpoprbppoprsipoprdipopr14popr15retM01_L09: ; expect the mov rax,{r15,rsi} the epilogs are the same, can they be collapsed tomovrax,rsi ; get less machine code?vzeroupperaddrsp,28poprbxpoprbppoprsipoprdipopr14popr15retM01_L10: ; this block should be common enough, so should be on the jump-root (see comment above)cmp[r15],r15b ; it's the Memmove-calllearcx,[r15+0C]leardx,[rsi+0C]movr8d,ebpaddr8,r8call qword ptr [7FFEFA7399F0]jmp near ptr M01_L01; Total bytes of code 383

So from code-layout one major difference to .NET 6 is that the call to System.Buffer.Memmove is moved out of the hot-path.
But I doubt that this allone is the cause for the regression.

I also wonder why vpblendvb is gone when using string.Replace in the benchmark from .NET-bits.
If I use a string.Replace-duplicated code for the benchmark, then it's emitted which is what I expect as 10d8a36 got merged on 2022-05-25.
But that shouldn't cause the regression either, as for .NET 6 the same series of vector-instruction are emitted.

The beginning of the method, right after the prolog, looks different between .NET 6 and .NET 7, although this PR didn't change anything here. I don't expect that this causes the regression, as with the given input the vectorized loop with 33 iterations should be dominant enough (just my feeling, maybe wrong).

So far the "static analysis", but I doubt this is enough.
With Intel VTune I see some results, but with my interpretation the conclusions are just the same as stated in this comment.
I hope some JIT experts can shed some light on this (and give some advices on how to investigate, as I'm eager to learn).

Machine code for .NET 6 (for reference)

; System.String.Replace(Char, Char)pushr15pushr14pushrdipushrsipushrbppushrbxsubrsp,28vzerouppermovrsi,rcxmovzxedi,dxmovzxebx,r8wcmpedi,ebxjne short M01_L00movrax,rsivzeroupperaddrsp,28poprbxpoprbppoprsipoprdipopr14popr15retM01_L00:learbp,[rsi+0C]movrcx,rbpmovr14d,[rsi+8]movr8d,r14dmovedx,edicall System.SpanHelpers.IndexOf(Char ByRef, Char, Int32)movr15d,eaxtestr15d,r15djge short M01_L01movrax,rsivzeroupperaddrsp,28poprbxpoprbppoprsipoprdipopr14popr15retM01_L01:movesi,r14dsubesi,r15dmovecx,r14dcall System.String.FastAllocateString(Int32)movr14,raxtestr15d,r15djle short M01_L02cmp[r14],r14dlearcx,[r14+0C]movrdx,rbpmovr8d,r15daddr8,r8call System.Buffer.Memmove(Byte ByRef, Byte ByRef, UIntPtr)M01_L02:movsxdrax,r15daddrax,raxaddrbp,raxcmp[r14],r14dleardx,[r14+0C]addrdx,raxcmpesi,10jl short M01_L04imuleax,edi,10001 vmovd xmm0,eaxvpbroadcastdymm0,xmm0imuleax,ebx,10001 vmovd xmm1,eaxvpbroadcastdymm1,xmm1M01_L03:vmovupdymm2,[rbp] vpcmpeqw ymm3,ymm2,ymm0vpandymm4,ymm1,ymm3vpandnymm2,ymm3,ymm2vporymm2,ymm4,ymm2vmovupd[rdx],ymm2addrbp,20addrdx,20addesi,0FFFFFFF0cmpesi,10jge short M01_L03M01_L04:testesi,esijle short M01_L08nop word ptr [rax+rax]M01_L05:movzxeax,word ptr [rbp]movrcx,rdxcmpeax,edije short M01_L06jmp short M01_L07M01_L06:moveax,ebxM01_L07:mov[rcx],axaddrbp,2addrdx,2decesitestesi,esijg short M01_L05M01_L08:movrax,r14vzeroupperaddrsp,28poprbxpoprbppoprsipoprdipopr14popr15ret; Total bytes of code 307

[AndyAyersMS]

(should we be surprised that disabling PGO didn't make any difference? perhaps it doesn't exercise this method? cc @AndyAyersMS )

Hard to say without looking deeper -- from the .NET 7 code above I would guess PGO is driving the code layout changes.

For the .NET 7 you can use DOTNET_JitDIsasm in BDN to obtain the jit disasm which will tell you if there was PGO found (at least for the root method).

[danmoseley]

I created #74771 and crudely pasted in the above discussion. Let's please continue there.