This repository was archived by the owner on Dec 18, 2018. It is now read-only.
Improve inlinability of libuv success checking #1059
Merged
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
|
This unprofitable inline
; Assembly listing for method Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:Check(int):int:this
; Emitting BLENDED_CODE for X64 CPU with SSE2
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
; V00 this [V00,T01] ( 3, 3 ) ref -> rcx this
; V01 arg1 [V01,T00] ( 4, 4 ) int -> rsi
; V02 loc0 [V02 ] ( 3, 2 ) ref -> [rsp+0x28] do-not-enreg[X] must-init addr-exposed ld-addr-op
; V03 OutArgs [V03 ] ( 1, 1 ) lclBlk (32) [rsp+0x00]
;
; Lcl frame size = 48
G_M62863_IG01:
push rsi
sub rsp, 48
xor rax, rax
mov qword ptr [rsp+28H], rax
mov esi, edx
G_M62863_IG02:
lea r8, bword ptr [rsp+28H]
mov edx, esi
call [Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:Check(int,byref):int:this]
mov rax, gword ptr [rsp+28H]
test rax, rax
jne SHORT G_M62863_IG05
G_M62863_IG03:
mov eax, esi
G_M62863_IG04:
add rsp, 48
pop rsi
ret
G_M62863_IG05:
mov rcx, gword ptr [rsp+28H]
call CORINFO_HELP_THROW
int3
; Total bytes of code 56, prolog size 12 for method Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:Check(int):int:this
; ============================================================Becomes this successful inline
; Assembly listing for method Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:ThrowIfErrored(int):this
; Emitting BLENDED_CODE for X64 CPU with SSE2
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
; V00 this [V00,T01] ( 3, 2 ) ref -> rcx this
; V01 arg1 [V01,T00] ( 4, 3 ) int -> rdx
; V02 OutArgs [V02 ] ( 1, 1 ) lclBlk (32) [rsp+0x00]
;
; Lcl frame size = 40
G_M39754_IG01:
sub rsp, 40
nop
G_M39754_IG02:
test edx, edx
jl SHORT G_M39754_IG04
G_M39754_IG03:
add rsp, 40
ret
G_M39754_IG04:
call [Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:ThrowError(int):this]
int3
; Total bytes of code 21, prolog size 5 for method Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:ThrowIfErrored(int):this |
|
This unprofitable inline
; Assembly listing for method Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:Check(int,byref):int:this
; Emitting BLENDED_CODE for X64 CPU with SSE2
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
; V00 this [V00,T02] ( 4, 3 ) ref -> rdi this
; V01 arg1 [V01,T01] ( 8, 6 ) int -> rsi
; V02 arg2 [V02,T03] ( 4, 3 ) byref -> rbx
; V03 loc0 [V03,T09] ( 2, 1 ) ref -> rbp
; V04 loc1 [V04,T10] ( 2, 1 ) ref -> rdi
; V05 tmp0 [V05,T00] ( 9, 9 ) ref -> r14
; V06 tmp1 [V06,T05] ( 3, 3 ) ref -> rax
; V07 tmp2 [V07,T04] ( 4, 4 ) ref -> rdi
; V08 tmp3 [V08,T06] ( 2, 2 ) ref -> rdx
; V09 tmp4 [V09,T11] ( 1, 1 ) ref -> rcx
; V10 tmp5 [V10,T07] ( 2, 2 ) ref -> r8
; V11 OutArgs [V11 ] ( 1, 1 ) lclBlk (32) [rsp+0x00]
; V12 cse0 [V12,T08] ( 3, 1.5) long -> r15
;
; Lcl frame size = 40
G_M62864_IG01:
push r15
push r14
push rdi
push rsi
push rbp
push rbx
sub rsp, 40
mov rdi, rcx
mov esi, edx
mov rbx, r8
G_M62864_IG02:
test esi, esi
jge G_M62864_IG04
mov rcx, rdi
mov edx, esi
call [Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:err_name(int):ref:this]
mov rbp, rax
mov rcx, rdi
mov edx, esi
call [Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:strerror(int):ref:this]
mov rdi, rax
mov ecx, 6
call [CORINFO_HELP_READYTORUN_NEWARR_1]
mov r14, rax
mov r8, qword ptr [(reloc)]
mov r8, gword ptr [r8]
mov rcx, r14
xor edx, edx
call [CORINFO_HELP_ARRADDR_ST]
call [CORINFO_HELP_READYTORUN_NEW]
mov rcx, r14
mov dword ptr [rax+8], esi
mov r8, rax
mov rcx, r14
mov edx, 1
call [CORINFO_HELP_ARRADDR_ST]
mov r15, qword ptr [(reloc)]
mov r8, gword ptr [r15]
mov rcx, r14
mov edx, 2
call [CORINFO_HELP_ARRADDR_ST]
mov rcx, r14
mov r8, rbp
mov edx, 3
call [CORINFO_HELP_ARRADDR_ST]
mov r8, gword ptr [r15]
mov rcx, r14
mov edx, 4
call [CORINFO_HELP_ARRADDR_ST]
mov rcx, r14
mov r8, rdi
mov edx, 5
call [CORINFO_HELP_ARRADDR_ST]
call [CORINFO_HELP_READYTORUN_NEW]
mov rdi, rax
mov rcx, r14
call [System.String:Concat(ref):ref]
mov rdx, rax
mov rcx, rdi
call [System.Exception:.ctor(ref):this]
G_M62864_IG03:
mov rdx, qword ptr [(reloc)]
mov dword ptr [rdi+rdx], esi
mov rcx, rbx
mov rdx, rdi
call [CORINFO_HELP_CHECKED_ASSIGN_REF]
jmp SHORT G_M62864_IG05
G_M62864_IG04:
xor rax, rax
mov gword ptr [rbx], rax
G_M62864_IG05:
mov eax, esi
G_M62864_IG06:
add rsp, 40
pop rbx
pop rbp
pop rsi
pop rdi
pop r14
pop r15
ret
; Total bytes of code 269, prolog size 12 for method Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:Check(int,byref):int:thisBecomes this successful inline
; Assembly listing for method Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:Check(int,byref):this
; Emitting BLENDED_CODE for X64 CPU with SSE2
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
; V00 this [V00,T02] ( 3, 2.5) ref -> rcx this
; V01 arg1 [V01,T00] ( 4, 3.5) int -> rdx
; V02 arg2 [V02,T01] ( 3, 3 ) byref -> r8
; V03 tmp0 [V03,T03] ( 3, 2 ) byref -> rsi
; V04 tmp1 [V04,T04] ( 3, 2 ) byref -> rsi
; V05 tmp2 [V05,T05] ( 3, 2 ) ref -> rdx
; V06 OutArgs [V06 ] ( 1, 1 ) lclBlk (32) [rsp+0x00]
;
; Lcl frame size = 32
G_M62863_IG01:
push rsi
sub rsp, 32
G_M62863_IG02:
mov rsi, r8
test edx, edx
jl SHORT G_M62863_IG03
xor rdx, rdx
jmp SHORT G_M62863_IG04
G_M62863_IG03:
call [Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:GetError(int):ref:this]
mov rdx, rax
G_M62863_IG04:
mov rcx, rsi
call [CORINFO_HELP_CHECKED_ASSIGN_REF]
nop
G_M62863_IG05:
add rsp, 32
pop rsi
ret
; Total bytes of code 41, prolog size 5 for method Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:Check(int,byref):this |
|
Changed dasm file Microsoft.AspNetCore.Server.Kestrel-Check.dasm.txt 4.4MB |
|
LGTM. 🚀 |
|
@benaadams Awesome change. Before we merge, can you add comments documenting that the code is factored that way to benefit from inlining? I want to avoid having someone change this in the future because they're not aware of what is being accomplished by having the code written this way. |
| { | ||
| return _uv.try_write(this, new[] { buf }, 1); | ||
| _uv.try_write(this, new[] { buf }, 1); |
There was a problem hiding this comment.
TryWrite returns the number of bytes written to a socket or an error.
I don't think that removing the returned value is really something one wants to do with this API. It is currently not used, but in the future, if one were to use it, one would check how much of the buffer would have been written by using the returned value and eventually queue the rest of the buffer for writing with a normal Write call.
There was a problem hiding this comment.
Returning value for try_write now
|
@CesarBS added comments |
|
@benaadams Thanks! 🚢 |
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.
Reviewing #1058
Currently the throwing
int Check(int statusCode)and the non throwingint Check(int statusCode, out Exception error)are non-inlinable functions with the second being fairly large.As one of these is called on every libuv call it would be better to have them much trimmer and inlinable; especially for the common success case.
[FAILED: unprofitable inline] Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:Check(int,byref):int:this[FAILED: unprofitable inline] Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.Libuv:Check(int):int:thisThis changes the throwing check to
ThrowIfErrored, which is what it does, and drops the return type from both (which nothing does anything with) as well as delegating the rarely taken error path to other functions to make the two functions much smaller and inlinable.Note: Need coreclr post dotnet/coreclr#6103 for the magic
FAILED: does not return; though will still work similarly, just might also inline the throw call./cc @halter73 @CesarBS @davidfowl