Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Inliner: enable inlining of methods with conditional throws #8038

Merged
merged 2 commits into from
Nov 9, 2016

Conversation

AndyAyersMS
Copy link
Member

Remove inlining limitation for methods that return values and have
conditional throws. This limitation was most likely a vestige of an
older inlining implementation that did not break trees at inline
call sites.

Also removed the now-unused observation and the seenConditionalJump
member variable. Merged ifdef blocks in impInit.

Ran full desktop testing, no issues.

Enables a handful of inlines in the various code size suites. For the
most part these slightly increase code size but can often shorten the
non-EH paths in the code.

Remove inlining limitation for methods that return values and have
conditional throws. This limitation was most likely a vestige of an
older inlining implementation that did not break trees at inline
call sites.

Also removed the now-unused observation and the `seenConditionalJump`
member variable. Merged ifdef blocks in `impInit`.

Ran full desktop testing, no issues.

Enables a handful of inlines in the various code size suites. For the
most part these slightly increase code size but can often shorten the
non-EH paths in the code.
@AndyAyersMS
Copy link
Member Author

cc @dotnet/jit-contrib

Output from jit-diff:

 bytes of diff: 1381 (0.02 % of base)
    diff is a regression.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
         994 : System.Private.CoreLib.dasm (0.03 % of base)
         330 : System.Net.Sockets.dasm (0.19 % of base)
          57 : System.Runtime.Extensions.dasm (0.07 % of base)
3 total files with size differences.
Top method regessions by size (bytes):
         191 : System.Private.CoreLib.dasm - mincore:GetMessage(long,int):ref
         164 : System.Private.CoreLib.dasm - mincore:TryGetErrorMessage(long,int,ref,byref):bool
         154 : System.Private.CoreLib.dasm - System.Guid:TryParseGuidWithNoStyle(ref,byref):bool
         150 : System.Private.CoreLib.dasm - System.Globalization.DateTimeFormatInfo:InsertAtCurrentHashNode(ref,ref,char,int,int,int,int,int):this
         132 : System.Private.CoreLib.dasm - System.Threading.ManualResetEventSlim:Wait(int,struct):bool:this
13 total methods with size differences.

Example code diff:

;;; Assembly listing for method System.AppDomain:ExecuteAssembly(ref):int:this
;;; Before

G_M56995_IG01:
       sub      rsp, 40
       nop      
G_M56995_IG02:
       xor      r8d, r8d
       mov      dword ptr [rsp+20H], r8d
       xor      r8, r8
       xor      r9, r9
       call     [System.AppDomain:ExecuteAssembly(ref,ref,ref,int):int:this]
       nop      
G_M56995_IG03:
       add      rsp, 40
       ret  

;;; After

G_M56995_IG01:
       push     rsi
       sub      rsp, 32
G_M56995_IG02:
       test     rdx, rdx
       je       SHORT G_M56995_IG05
G_M56995_IG03:
       xor      eax, eax
G_M56995_IG04:
       add      rsp, 32
       pop      rsi
       ret      
G_M56995_IG05:
       call     [CORINFO_HELP_READYTORUN_NEW]
       mov      rsi, rax
       mov      ecx, 425
       call     CORINFO_HELP_STRCNS_CURRENT_MODULE
       mov      rdx, rax
       mov      rcx, rsi
       call     [System.ArgumentNullException:.ctor(ref):this]
       mov      rcx, rsi
       call     CORINFO_HELP_THROW
       int3

@omariom
Copy link

omariom commented Nov 8, 2016

So now methods like the following will be inlined?

private int GetLength(string str)
{
   if (str == null)
      throw new ArgumentNullException();

   return str.Length:
}

@AndyAyersMS
Copy link
Member Author

@omariom the conditional throw will no longer block inlining, so a simple method like this should get inlined.

@AndyAyersMS
Copy link
Member Author

@dotnet/jit-contrib anyone up for a review? It is a pretty small change...

@erozenfeld
Copy link
Member

LGTM

@AndyAyersMS
Copy link
Member Author

Ignoring the jit diff failure.

@AndyAyersMS AndyAyersMS merged commit e76da56 into dotnet:master Nov 9, 2016
@AndyAyersMS AndyAyersMS deleted the InlineConditionalThrows branch November 9, 2016 23:03
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
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.

6 participants