Improve String.Substring performance #62577
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
ghost
added
the
community-contribution
|
Using Span.Slice guard clauses for perf. Regressions I believe are due to benchmark issues. In any case, is changing guard clauses to this an acceptable change? Should exception messages be preserved? BenchmarkDotNet=v0.13.1.1620-nightly, OS=Windows 10.0.19044.1348 (21H2)
AMD Ryzen 9 5950X, 1 CPU, 32 logical and 16 physical cores
.NET SDK=7.0.100-alpha.1.21568.2
[Host] : .NET 6.0.0 (6.0.21.48005), X64 RyuJIT
Job-ICQHYC : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT
Job-KPKVVD : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
|
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsWIP just to get the ball rolling regarding discussion of changing exception messages or similar. cc @adamsitnik @GrabYourPitchforks @stephentoub as discussed in #60463 Change to BenchmarkDotNet=v0.13.1.1620-nightly, OS=Windows 10.0.19044.1348 (21H2)
AMD Ryzen 9 5950X, 1 CPU, 32 logical and 16 physical cores
.NET SDK=7.0.100-alpha.1.21568.2
[Host] : .NET 6.0.0 (6.0.21.48005), X64 RyuJIT
Job-FWUOEW : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT
Job-HPGSVV : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
|
src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs
Outdated
Show resolved
Hide resolved
|
@GrabYourPitchforks tried to address feedback. Still don't know what to do about getting reliable benchmark tests. These do not show expected results currently but are also volatile. Substring(0) still faster of course. Tried running Let me know, what next steps you guys think should be. |
jkotas
reviewed
Dec 11, 2021
src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs
Outdated
Show resolved
Hide resolved
|
Cleaned up. Benchmarks below, only significant improvements for when returning same string or empty. Regressions are often due to variance in benchmarks. Main point probably is throw code is separated out reducing code size of the method. I have included the cost of new'ing a similar string for comparison here. Substring(int)
Substring(int,int)
ASMM; System.Tests.Perf_String_Substring_IntInt.Substring()
sub rsp,28
mov r8,[rcx+8]
mov rcx,1B31ACA8048
mov rcx,[rcx]
mov edx,[r8+8]
mov r8d,[r8+0C]
call System.String.Substring(Int32, Int32)
nop
add rsp,28
ret
; Total bytes of code 40; System.String.Substring(Int32, Int32)
push rdi
push rsi
push rbx
sub rsp,20
mov rdi,rcx
mov esi,edx
test esi,esi
jl near ptr M01_L03
mov ebx,[rdi+8]
cmp ebx,esi
jl near ptr M01_L04
test r8d,r8d
jl near ptr M01_L05
mov ecx,ebx
sub ecx,r8d
cmp ecx,esi
jl near ptr M01_L06
test r8d,r8d
je short M01_L02
test esi,esi
je short M01_L01
M01_L00:
mov ecx,r8d
call System.String.FastAllocateString(Int32)
mov rbx,rax
mov r8d,[rbx+8]
lea rcx,[rbx+0C]
add rdi,0C
mov edx,esi
lea rdx,[rdi+rdx*2]
add r8,r8
call System.Buffer.Memmove(Byte ByRef, Byte ByRef, UIntPtr)
mov rax,rbx
add rsp,20
pop rbx
pop rsi
pop rdi
ret
M01_L01:
cmp ebx,r8d
jne short M01_L00
mov rax,rdi
add rsp,20
pop rbx
pop rsi
pop rdi
ret
M01_L02:
mov rax,1B31ACA3020
mov rax,[rax]
add rsp,20
pop rbx
pop rsi
pop rdi
ret
M01_L03:
mov rcx,offset MT_System.ArgumentOutOfRangeException
call CORINFO_HELP_NEWSFAST
mov rsi,rax
mov ecx,19A4
mov rdx,7FF7DA114000
call CORINFO_HELP_STRCNS
mov rdi,rax
mov ecx,0A855
mov rdx,7FF7DA114000
call CORINFO_HELP_STRCNS
mov rcx,rax
call System.SR.GetResourceString(System.String)
mov r8,rax
mov rdx,rdi
mov rcx,rsi
call System.ArgumentOutOfRangeException..ctor(System.String, System.String)
mov rcx,rsi
call CORINFO_HELP_THROW
M01_L04:
mov rcx,offset MT_System.ArgumentOutOfRangeException
call CORINFO_HELP_NEWSFAST
mov rsi,rax
mov ecx,19A4
mov rdx,7FF7DA114000
call CORINFO_HELP_STRCNS
mov rdi,rax
mov ecx,0A891
mov rdx,7FF7DA114000
call CORINFO_HELP_STRCNS
mov rcx,rax
call System.SR.GetResourceString(System.String)
mov r8,rax
mov rdx,rdi
mov rcx,rsi
call System.ArgumentOutOfRangeException..ctor(System.String, System.String)
mov rcx,rsi
call CORINFO_HELP_THROW
M01_L05:
mov rcx,offset MT_System.ArgumentOutOfRangeException
call CORINFO_HELP_NEWSFAST
mov rsi,rax
mov ecx,57
mov rdx,7FF7DA114000
call CORINFO_HELP_STRCNS
mov rdi,rax
mov ecx,0A519
mov rdx,7FF7DA114000
call CORINFO_HELP_STRCNS
mov rcx,rax
call System.SR.GetResourceString(System.String)
mov r8,rax
mov rdx,rdi
mov rcx,rsi
call System.ArgumentOutOfRangeException..ctor(System.String, System.String)
mov rcx,rsi
call CORINFO_HELP_THROW
M01_L06:
mov rcx,offset MT_System.ArgumentOutOfRangeException
call CORINFO_HELP_NEWSFAST
mov rsi,rax
mov ecx,57
mov rdx,7FF7DA114000
call CORINFO_HELP_STRCNS
mov rdi,rax
mov ecx,9EBD
mov rdx,7FF7DA114000
call CORINFO_HELP_STRCNS
mov rcx,rax
call System.SR.GetResourceString(System.String)
mov r8,rax
mov rdx,rdi
mov rcx,rsi
call System.ArgumentOutOfRangeException..ctor(System.String, System.String)
mov rcx,rsi
call CORINFO_HELP_THROW
int 3
; Total bytes of code 512PR; System.Tests.Perf_String_Substring_IntInt.Substring()
sub rsp,28
mov r8,[rcx+8]
mov rcx,1FA62268048
mov rcx,[rcx]
mov edx,[r8+8]
mov r8d,[r8+0C]
call System.String.Substring(Int32, Int32)
nop
add rsp,28
ret
; Total bytes of code 40; System.String.Substring(Int32, Int32)
push rdi
push rsi
push rbx
sub rsp,20
mov rsi,rcx
mov edi,edx
mov ecx,[rsi+8]
mov eax,edi
mov edx,r8d
add rax,rdx
mov edx,ecx
cmp rax,rdx
ja short M01_L02
test r8d,r8d
jne short M01_L00
mov rax,1FA62263020
mov rax,[rax]
add rsp,20
pop rbx
pop rsi
pop rdi
ret
M01_L00:
cmp r8d,ecx
jne short M01_L01
mov rax,rsi
add rsp,20
pop rbx
pop rsi
pop rdi
ret
M01_L01:
mov ecx,r8d
call System.String.FastAllocateString(Int32)
mov rbx,rax
mov r8d,[rbx+8]
lea rcx,[rbx+0C]
add rsi,0C
mov edx,edi
lea rdx,[rsi+rdx*2]
add r8,r8
call System.Buffer.Memmove(Byte ByRef, Byte ByRef, UIntPtr)
mov rax,rbx
add rsp,20
pop rbx
pop rsi
pop rdi
ret
M01_L02:
mov rcx,rsi
mov edx,edi
call System.String.ThrowSubstringArgumentOutRangeException(Int32, Int32)
int 3
; Total bytes of code 131 |
|
@GrabYourPitchforks should I close this or? |
deeprobin
approved these changes
Mar 14, 2022
|
I'm going to close this as the improvements are as you say marginal. Thank you for the attempt! |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP just to get the ball rolling regarding discussion of changing exception messages or similar. cc @adamsitnik @GrabYourPitchforks @stephentoub as discussed in #60463
Change to
Substring(int startIndex)improvements (note highly volatile). This comes with all the caveats of potential increases code size etc.