Remove some calls to wstrcpy by GrabYourPitchforks · Pull Request #32342 · dotnet/runtime

GrabYourPitchforks · 2020-02-15T00:02:20Z

Some modest codegen + perf improvements to existing string functions. For instance, calling the string(char[]) ctor:

Method	Toolchain	CharArrayLength	Mean	Error	StdDev	Median	Ratio	RatioSD
FromCharArrCtor	master	0	2.035 ns	0.1192 ns	0.0931 ns	2.018 ns	1.00	0.00
FromCharArrCtor	wstrcpy_2	0	1.651 ns	0.0824 ns	0.0643 ns	1.652 ns	0.81	0.05

FromCharArrCtor	master	4	10.552 ns	0.2164 ns	0.2024 ns	10.490 ns	1.00	0.00
FromCharArrCtor	wstrcpy_2	4	9.084 ns	0.2481 ns	0.2199 ns	9.081 ns	0.86	0.03

FromCharArrCtor	master	16	12.158 ns	0.2545 ns	0.2256 ns	12.099 ns	1.00	0.00
FromCharArrCtor	wstrcpy_2	16	10.760 ns	0.2939 ns	0.4308 ns	10.603 ns	0.90	0.05

FromCharArrCtor	master	71	24.151 ns	1.3100 ns	3.6299 ns	22.395 ns	1.00	0.00
FromCharArrCtor	wstrcpy_2	71	19.614 ns	0.4503 ns	0.4818 ns	19.516 ns	0.80	0.13

; -- CODEGEN BEFORE --

00007ff8`f97c9e80 57              push    rdi
00007ff8`f97c9e81 56              push    rsi
00007ff8`f97c9e82 53              push    rbx
00007ff8`f97c9e83 4883ec30        sub     rsp,30h
00007ff8`f97c9e87 33c0            xor     eax,eax
00007ff8`f97c9e89 4889442428      mov     qword ptr [rsp+28h],rax
00007ff8`f97c9e8e 4889442420      mov     qword ptr [rsp+20h],rax
00007ff8`f97c9e93 488bf2          mov     rsi,rdx
00007ff8`f97c9e96 4885f6          test    rsi,rsi ; if (value == null)
00007ff8`f97c9e99 7407            je      System_Private_CoreLib!System.String.Ctor(Char[])+0xffffffff`b417d9b2 (00007ff8`f97c9ea2)
00007ff8`f97c9e9b 8b7e08          mov     edi,dword ptr [rsi+8]
00007ff8`f97c9e9e 85ff            test    edi,edi ; if (value.Length == 0)
00007ff8`f97c9ea0 7515            jne     System_Private_CoreLib!System.String.Ctor(Char[])+0xffffffff`b417d9c7 (00007ff8`f97c9eb7)
00007ff8`f97c9ea2 48b86030905850020000 mov rax,25058903060h ; return string.Empty
00007ff8`f97c9eac 488b00          mov     rax,qword ptr [rax]
00007ff8`f97c9eaf 4883c430        add     rsp,30h
00007ff8`f97c9eb3 5b              pop     rbx
00007ff8`f97c9eb4 5e              pop     rsi
00007ff8`f97c9eb5 5f              pop     rdi
00007ff8`f97c9eb6 c3              ret
00007ff8`f97c9eb7 8bcf            mov     ecx,edi
00007ff8`f97c9eb9 e8caa9d8ff      call    <FastAllocateString>
00007ff8`f97c9ebe 488bd8          mov     rbx,rax
00007ff8`f97c9ec1 391b            cmp     dword ptr [rbx],ebx ; null check on return value from FastAllocateString
00007ff8`f97c9ec3 4c8d430c        lea     r8,[rbx+0Ch] ; &return._firstChar
00007ff8`f97c9ec7 4c89442428      mov     qword ptr [rsp+28h],r8
00007ff8`f97c9ecc 488b4c2428      mov     rcx,qword ptr [rsp+28h]
00007ff8`f97c9ed1 4889742420      mov     qword ptr [rsp+20h],rsi
00007ff8`f97c9ed6 4c8b442420      mov     r8,qword ptr [rsp+20h]
00007ff8`f97c9edb 4183780800      cmp     dword ptr [r8+8],0
00007ff8`f97c9ee0 7504            jne     System_Private_CoreLib!System.String.Ctor(Char[])+0xffffffff`b417d9f6 (00007ff8`f97c9ee6)
00007ff8`f97c9ee2 33d2            xor     edx,edx
00007ff8`f97c9ee4 eb14            jmp     System_Private_CoreLib!System.String.Ctor(Char[])+0xffffffff`b417da0a (00007ff8`f97c9efa)
00007ff8`f97c9ee6 488b542420      mov     rdx,qword ptr [rsp+20h]
00007ff8`f97c9eeb 837a0800        cmp     dword ptr [rdx+8],0
00007ff8`f97c9eef 762c            jbe     System_Private_CoreLib!System.String.Ctor(Char[])+0xffffffff`b417da2d (00007ff8`f97c9f1d)
00007ff8`f97c9ef1 488b542420      mov     rdx,qword ptr [rsp+20h]
00007ff8`f97c9ef6 4883c210        add     rdx,10h
00007ff8`f97c9efa d1e7            shl     edi,1
00007ff8`f97c9efc 448bc7          mov     r8d,edi
00007ff8`f97c9eff e8b407d9ff      call    <Memmove> ; wstrcpy was inlined into caller
00007ff8`f97c9f04 33c0            xor     eax,eax
00007ff8`f97c9f06 4889442428      mov     qword ptr [rsp+28h],rax
00007ff8`f97c9f0b 33c0            xor     eax,eax
00007ff8`f97c9f0d 4889442420      mov     qword ptr [rsp+20h],rax
00007ff8`f97c9f12 488bc3          mov     rax,rbx
00007ff8`f97c9f15 4883c430        add     rsp,30h
00007ff8`f97c9f19 5b              pop     rbx
00007ff8`f97c9f1a 5e              pop     rsi
00007ff8`f97c9f1b 5f              pop     rdi
00007ff8`f97c9f1c c3              ret
00007ff8`f97c9f1d e88ecca25f      call    CoreCLR!GetCLRRuntimeHost+0x64390 (00007ff9`591f6bb0)
00007ff8`f97c9f22 cc              int     3

; -- CODEGEN AFTER --

00007ff8`f97a9e80 57              push    rdi
00007ff8`f97a9e81 56              push    rsi
00007ff8`f97a9e82 4883ec28        sub     rsp,28h
00007ff8`f97a9e86 488bf2          mov     rsi,rdx
00007ff8`f97a9e89 4885f6          test    rsi,rsi ; if (value == null)
00007ff8`f97a9e8c 7407            je      System_Private_CoreLib!System.String.Ctor(Char[])+0xffffffff`ae5edc05 (00007ff8`f97a9e95)
00007ff8`f97a9e8e 8b4e08          mov     ecx,dword ptr [rsi+8]
00007ff8`f97a9e91 85c9            test    ecx,ecx ; if (value.Length == 0)
00007ff8`f97a9e93 7514            jne     System_Private_CoreLib!System.String.Ctor(Char[])+0xffffffff`ae5edc19 (00007ff8`f97a9ea9)
00007ff8`f97a9e95 48b860302154a4020000 mov rax,2A454213060h ; return string.Empty
00007ff8`f97a9e9f 488b00          mov     rax,qword ptr [rax]
00007ff8`f97a9ea2 4883c428        add     rsp,28h
00007ff8`f97a9ea6 5e              pop     rsi
00007ff8`f97a9ea7 5f              pop     rdi
00007ff8`f97a9ea8 c3              ret
00007ff8`f97a9ea9 e8daa9d8ff      call    <FastAllocateString>
00007ff8`f97a9eae 488bf8          mov     rdi,rax
00007ff8`f97a9eb1 448b4708        mov     r8d,dword ptr [rdi+8] ; no extra indirection to check for null 'result'
00007ff8`f97a9eb5 488d4f0c        lea     rcx,[rdi+0Ch]
00007ff8`f97a9eb9 488d5610        lea     rdx,[rsi+10h]
00007ff8`f97a9ebd 49d1e0          shl     r8,1
00007ff8`f97a9ec0 e80308d9ff      call    <Memmove(byte)> ; Memmove(T) was inlined into caller
00007ff8`f97a9ec5 488bc7          mov     rax,rdi
00007ff8`f97a9ec8 4883c428        add     rsp,28h
00007ff8`f97a9ecc 5e              pop     rsi
00007ff8`f97a9ecd 5f              pop     rdi
00007ff8`f97a9ece c3              ret

I didn't remove all calls to wstrcpy because there were some places (like below) where the pointer math would have made the calls to Unsafe.Add(...) unreadable.

runtime/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs

Lines 946 to 953 in b95e523

    
           fixed (char* src = &_firstChar) 
        
           { 
        
               fixed (char* dst = &result._firstChar) 
        
               { 
        
                   wstrcpy(dst, src, startIndex); 
        
                   wstrcpy(dst + startIndex, src + startIndex + count, newLength - startIndex); 
        
               } 
        
           }

src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs

jkotas · 2020-02-15T06:31:50Z

didn't remove all calls to wstrcpy

I think changing the remaining two wstrcpy uses in string.cs would be fine.

I guess that you have not touched Copy because of we consider it to be obsolete ... but we still carry the slightly bigger code around and there is value in consistency.

GrabYourPitchforks · 2020-02-21T00:32:58Z

Tests are flaky on my machine for some reason (probably some stale files in the build) - so relying on the CI to catch any problems here.

Remove some calls to wstrcpy

d8f0685

GrabYourPitchforks added the area-System.Runtime label Feb 15, 2020

AaronRobinsonMSFT approved these changes Feb 15, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs Show resolved Hide resolved

AaronRobinsonMSFT added the tenet-performance Performance related issue label Feb 15, 2020

stephentoub approved these changes Feb 15, 2020

View reviewed changes

GrabYourPitchforks added 2 commits February 20, 2020 15:47

Merge remote-tracking branch 'origin/master' into wstrcpy_2

417fe96

PR feedback

42a7d6f

jkotas approved these changes Feb 21, 2020

View reviewed changes

GrabYourPitchforks merged commit 70c1e9d into dotnet:master Feb 21, 2020

GrabYourPitchforks deleted the wstrcpy_2 branch February 21, 2020 04:54

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some calls to wstrcpy#32342

Remove some calls to wstrcpy#32342
GrabYourPitchforks merged 3 commits intodotnet:masterfrom
GrabYourPitchforks:wstrcpy_2

GrabYourPitchforks commented Feb 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkotas commented Feb 15, 2020

Uh oh!

GrabYourPitchforks commented Feb 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	fixed (char* src = &_firstChar)
	{
	fixed (char* dst = &result._firstChar)
	{
	wstrcpy(dst, src, startIndex);
	wstrcpy(dst + startIndex, src + startIndex + count, newLength - startIndex);
	}
	}

Conversation

GrabYourPitchforks commented Feb 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkotas commented Feb 15, 2020

Uh oh!

GrabYourPitchforks commented Feb 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants