Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Throw when setting OnStarting after response has already started (#806). #1038

Merged
merged 2 commits into from
Aug 8, 2016

Conversation

cesarblum
Copy link
Contributor

@mikeharder
Copy link
Contributor

Would it be worthwhile to add a functional test as well, showing how customer code would set OnStarting()?

@@ -382,6 +382,11 @@ public void OnStarting(Func<object, Task> callback, object state)
{
lock (_onStartingSync)
{
if (HasResponseStarted)
{
ThrowResponseAlreadyStartedException(nameof(OnStarting));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use an enum rather than a string; string version is additional calls, better to push it out to the infrequently called function. (related background info dotnet/coreclr#6634 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You did that in #962, right? I can change this if we take it 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get a move on 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benaadams Could you elaborate on your first comment about using an enum? Not sure what's going on there.

Copy link
Contributor

@benaadams benaadams Aug 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with the string it inserts a load string call then calls the function

mov      ecx, 0x498F
call     CORINFO_HELP_STRCNS_CURRENT_MODULE
mov      rcx, rax
call     Frame:ThrowResponseAlreadyStartedException(ref)

With the enum it just sets a value

mov      ecx, 3
call     Frame:ThrowResponseAlreadyStartedException(int)

It does push the load string off to Frame:ThrowResponseAlreadyStartedException as the string still needs to get loaded; but that doesn't really ever get called, so the main function is more trim and the instruction cache hotter.

Copy link
Contributor

@benaadams benaadams Aug 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also hopefully it will also soon consistently move those calls to the bottom of the function as seen in this example (from dotnet/coreclr#6634 (comment))

Jit inline decisions (decides not to inline the functions that throw [no-return])

Inlines into 06002A67 ArraySegment`1:.ctor(ref,int,int):this
  [0 IL=0004 TR=000067 06000C7B] [FAILED: does not return] ThrowHelper:ThrowArgumentNullException(int)
  [0 IL=0016 TR=000061 06000C69] [FAILED: does not return] ThrowHelper:ThrowArgumentOutOfRangeException(int,int)
  [0 IL=0028 TR=000054 06000C69] [FAILED: does not return] ThrowHelper:ThrowArgumentOutOfRangeException(int,int)
  [0 IL=0043 TR=000048 06000C77] [FAILED: does not return] ThrowHelper:ThrowArgumentException(int)
Budget: initialTime=270, finalTime=270, initialBudget=2700, currentBudget=2700
Budget: initialSize=1727, finalSize=1727

Moves the calling of the throw functions out of the flow, even though they were at the start to a section called "cold code" (which is also does for straight throws, but they have more preamble than a function call)

; Assembly listing for method ArraySegment`1:.ctor(ref,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,T00] (  5,   5  )   byref  ->  rsi         this
;* V01 TypeCtx      [V01    ] (  0,   0  )    long  ->  zero-ref   
;  V02 arg1         [V02,T01] (  5,   5  )     ref  ->   r8        
;  V03 arg2         [V03,T02] (  5,   5  )     int  ->  rdi        
;  V04 arg3         [V04,T03] (  3,   3  )     int  ->  rbx        
;  V05 loc0         [V05    ] (  1,   1  )  lclBlk (32) [rsp+0x00]  
;
; Lcl frame size = 32
G_M41510_IG01:
       push     rdi
       push     rsi
       push     rbx
       sub      rsp, 32
       mov      rsi, rcx
       mov      edi, r9d
       mov      ebx, dword ptr [rsp+60H]
G_M41510_IG02:
       test     r8, r8
       je       G_M41510_IG08
G_M41510_IG03:
       test     edi, edi
       jl       G_M41510_IG09
G_M41510_IG04:
       test     ebx, ebx
       jl       G_M41510_IG10
G_M41510_IG05:
       mov      edx, dword ptr [r8+8]
       sub      edx, edi
       cmp      edx, ebx
       jl       G_M41510_IG11
G_M41510_IG06:
       mov      rcx, rsi
       mov      rdx, r8
       call     CORINFO_HELP_CHECKED_ASSIGN_REF
       mov      dword ptr [rsi+8], edi
       mov      dword ptr [rsi+12], ebx
G_M41510_IG07:
       add      rsp, 32
       pop      rbx
       pop      rsi
       pop      rdi
       ret      
************** Beginning of cold code **************
G_M41510_IG08:
       mov      ecx, 3
       call     ThrowHelper:ThrowArgumentNullException(int)
G_M41510_IG09:
       mov      ecx, 41
       mov      edx, 4
       call     ThrowHelper:ThrowArgumentOutOfRangeException(int,int)
G_M41510_IG10:
       mov      ecx, 16
       mov      edx, 4
       call     ThrowHelper:ThrowArgumentOutOfRangeException(int,int)
G_M41510_IG11:
       mov      ecx, 23
       call     ThrowHelper:ThrowArgumentException(int)
       int3     
; Total bytes of code 132, prolog size 7 for method ArraySegment`1:.ctor(ref,int,int):this

@mikeharder
Copy link
Contributor

:shipit: from me, though you might want to review @benaadams comment.

@davidfowl
Copy link
Member

:shipit:

@cesarblum cesarblum merged commit f2fa8b5 into dev Aug 8, 2016
@cesarblum cesarblum deleted the cesarbs/throw-onstarting-after-response-start branch August 8, 2016 19:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants