RyuJIT: Don't inline calls inside rarely used basic-blocks

[EgorBo]

Let's not inline calls inside rarely used basic blocks, e.g. blocks with throw (we already don't do it inside catch and filter ones).
An example with throw:

Exception GetMyException() => new ArgumentException("should not be zero");

int DoWork(int x)
{
    if (x == 0)
        throw GetMyException();

    return x * 1000;
}

Current Codegen (inliner inlines GetMyException()):

; Method Program:DoWork(int):int:this
G_M62105_IG01:
       push     rsi
       sub      rsp, 32
						;; bbWeight=1    PerfScore 1.25
G_M62105_IG02:
       test     edx, edx
       je       SHORT G_M62105_IG05
						;; bbWeight=1    PerfScore 1.25
G_M62105_IG03:
       imul     eax, edx, 0x3E8
						;; bbWeight=1    PerfScore 2.00
G_M62105_IG04:
       add      rsp, 32
       pop      rsi
       ret      
						;; bbWeight=1    PerfScore 1.75
G_M62105_IG05: ; <--- cold block
       mov      rcx, 0xD1FFAB1E
       call     CORINFO_HELP_NEWSFAST
       mov      rsi, rax
       mov      ecx, 7
       mov      rdx, 0xD1FFAB1E
       call     CORINFO_HELP_STRCNS
       mov      rdx, rax
       mov      rcx, rsi
       call     System.ArgumentException:.ctor(System.String):this
       mov      rcx, rsi
       call     CORINFO_HELP_THROW
       int3     
						;; bbWeight=0    PerfScore 0.00
; Total bytes of code: 79

Expected Codegen (my prototype):

G_M62105_IG01:
       sub      rsp, 40
						;; bbWeight=1    PerfScore 0.25
G_M62105_IG02:
       test     edx, edx
       je       SHORT G_M62105_IG05
						;; bbWeight=1    PerfScore 1.25
G_M62105_IG03:
       imul     eax, edx, 0x3E8
						;; bbWeight=1    PerfScore 2.00
G_M62105_IG04:
       add      rsp, 40
       ret      
						;; bbWeight=1    PerfScore 1.25
G_M62105_IG05:
       call     Program:GetMyException():System.Exception:this
       mov      rcx, rax
       call     CORINFO_HELP_THROW
       int3     
						;; bbWeight=0    PerfScore 0.00
; Total bytes of code: 33

-46 bytes of code.

It shouldn't also inline various SR.get_...() calls (used a lot across the BCL) string properties for exceptions, e.g.:

int DoWork(int x)
{
    if (x == 0)
        throw new ArgumentNullException(SR.InvalidArgument); // get_InvalidArgument() is inlined here

    return x * 1000;
}

jit-diff from my prototype (Release, -pmi -f):

C:\prj>jit-diff diff --output C:\prj\jitdiffs -f --core_root C:\prj\runtime-1\artifacts\tests\coreclr\Windows_NT.x64.Release\Tests\Core_Root --base C:\prj\runtime-1\artifacts\bin\coreclr\Windows_NT.x64.Checked_base --diff C:\prj\runtime-1\artifacts\bin\coreclr\Windows_NT.x64.Checked --pmi
Beginning PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies
| Finished 267/267 Base 267/267 Diff [444.3 sec]
Completed PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies in 444.39s
Diffs (if any) can be viewed by comparing: C:\prj\jitdiffs\dasmset_25\base C:\prj\jitdiffs\dasmset_25\diff
Analyzing CodeSize diffs...
Found 166 files with textual diffs.
PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for  default jit
Summary of Code Size diffs:
(Lower is better)
Total bytes of diff: -701786 (-1.39% of base)
    diff is an improvement.
Top file regressions (bytes):
         400 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.01% of base)
         110 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)
          21 : xunit.core.dasm (0.03% of base)
          21 : xunit.execution.dotnet.dasm (0.01% of base)
          14 : Microsoft.Extensions.FileProviders.Abstractions.dasm (1.63% of base)
           7 : Microsoft.Extensions.FileProviders.Physical.dasm (0.05% of base)
           5 : System.Net.WebProxy.dasm (0.17% of base)
Top file improvements (bytes):
     -103711 : System.Private.CoreLib.dasm (-2.31% of base)
      -64416 : System.Data.Common.dasm (-4.34% of base)
      -55982 : System.Private.Xml.dasm (-1.57% of base)
      -51052 : FSharp.Core.dasm (-1.55% of base)
      -33522 : System.Linq.Queryable.dasm (-12.00% of base)
      -29048 : System.Linq.Expressions.dasm (-3.80% of base)
      -21736 : System.Collections.dasm (-4.80% of base)
      -18318 : System.Private.DataContractSerialization.dasm (-2.43% of base)
      -13409 : Microsoft.VisualBasic.Core.dasm (-2.82% of base)
      -12690 : System.DirectoryServices.dasm (-2.92% of base)
      -12608 : System.Configuration.ConfigurationManager.dasm (-3.63% of base)
      -12391 : Newtonsoft.Json.dasm (-1.43% of base)
      -12161 : System.Security.Cryptography.Algorithms.dasm (-3.78% of base)
      -11517 : System.Threading.Tasks.Dataflow.dasm (-1.36% of base)
      -11124 : System.Collections.Concurrent.dasm (-3.33% of base)
       -9320 : System.Data.OleDb.dasm (-3.14% of base)
       -8294 : System.Net.Http.dasm (-1.12% of base)
       -6524 : System.DirectoryServices.AccountManagement.dasm (-1.81% of base)
       -6445 : System.ComponentModel.Composition.dasm (-1.94% of base)
       -6433 : System.Security.Cryptography.Pkcs.dasm (-1.64% of base)
166 total files with Code Size differences (159 improved, 7 regressed), 101 unchanged.
Top method regressions (bytes):
         129 (17.32% of base) : System.DirectoryServices.dasm - DomainController:GetRoles():ArrayList:this
          83 ( 7.94% of base) : System.DirectoryServices.dasm - Forest:GetSites():ArrayList:this
          76 ( 4.26% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LookupPosition:GetFirstExcludedToken(StatementSyntax):SyntaxToken
          53 ( 6.44% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SyntaxNodeExtensions:QueryClauseKeywordOrRangeVariableIdentifier(VisualBasicSyntaxNode):SyntaxToken
          45 ( 3.42% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:BindSubsequentQueryOperators(BoundQueryClauseBase,Enumerator,DiagnosticBag):BoundQueryClauseBase:this
          43 ( 3.74% of base) : System.DirectoryServices.dasm - DomainController:GetDomainControllerInfo():this
          37 (17.96% of base) : System.Data.Common.dasm - DataRow:SetParentRowToDBNull(DataRelation):this
          35 (350.00% of base) : System.Configuration.ConfigurationManager.dasm - ConfigurationSectionCollection:.ctor(SerializationInfo,StreamingContext):this
          35 (350.00% of base) : System.Management.dasm - ManagementNamedValueCollection:.ctor(SerializationInfo,StreamingContext):this
          35 (350.00% of base) : System.Net.Requests.dasm - FileWebResponse:.ctor(SerializationInfo,StreamingContext):this
          35 (250.00% of base) : System.Net.Requests.dasm - HttpWebResponse:.ctor(SerializationInfo,StreamingContext):this
          35 (350.00% of base) : System.Security.Cryptography.X509Certificates.dasm - X509Certificate2:.ctor(SerializationInfo,StreamingContext):this
          33 (31.73% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:GetQueryLambdaParameterNameRight(ImmutableArray`1):String
          32 ( 7.24% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:BindXmlContent(XmlNodeSyntax,XmlElementRootInfo,DiagnosticBag):BoundExpression:this
          31 ( 6.70% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - DocumentationCommentCrefBinder:CollectCrefNameSymbolsStrict(TypeSyntax,int,Dictionary`2,ArrayBuilder`1,bool,byref):this
          31 ( 3.73% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - DeclarationTreeBuilder:GetModifiers(SyntaxTokenList):int
          30 ( 0.83% of base) : Microsoft.CodeAnalysis.CSharp.dasm - SourceNamedTypeSymbol:MakeTypeParameters(DiagnosticBag):ImmutableArray`1:this
          30 ( 1.19% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:BindArgumentsAndNames(ImmutableArray`1,byref,byref,byref,DiagnosticBag):this
          29 ( 9.76% of base) : System.Private.Xml.dasm - ReflectionXmlSerializationWriter:WriteArray(Object,Object,ref,TextAccessor,ChoiceIdentifierAccessor,TypeDesc):this
          28 ( 1.21% of base) : System.Numerics.Tensors.dasm - DenseTensor`1:.ctor(Memory`1,ReadOnlySpan`1,bool):this (7 methods)
Top method improvements (bytes):
       -4214 (-31.03% of base) : System.Linq.Queryable.dasm - EnumerableQuery`1:System.Linq.IQueryProvider.CreateQuery(Expression):IQueryable`1:this (49 methods)
       -2058 (-12.05% of base) : System.Data.Common.dasm - EnumerableRowCollection`1:AddSortExpression(Func`2,IComparer`1,bool,bool):this (49 methods)
       -1659 (-29.15% of base) : System.Collections.Concurrent.dasm - BlockingCollection`1:ValidateCollectionsArray(ref,bool) (7 methods)
       -1554 (-25.99% of base) : System.Private.Xml.dasm - FunctionInfo`1:CastArguments(IList`1,String,XPathQilFactory):this (7 methods)
       -1470 (-13.35% of base) : System.Linq.Queryable.dasm - Queryable:Sum(IQueryable`1,Expression`1):Nullable`1 (35 methods)
       -1470 (-13.26% of base) : System.Linq.Queryable.dasm - Queryable:Average(IQueryable`1,Expression`1):Nullable`1 (35 methods)
       -1456 (-19.35% of base) : System.Linq.Parallel.dasm - PartitionerQueryOperatorResults:GivePartitionedStream(IPartitionedStreamRecipient`1):this (7 methods)
       -1358 (-14.67% of base) : System.Configuration.ConfigurationManager.dasm - ConfigurationElement:DeserializeElement(XmlReader,bool):this
       -1309 (-28.33% of base) : System.Data.Common.dasm - ExpressionParser:Parse():ExpressionNode:this
       -1232 (-27.72% of base) : FSharp.Core.dasm - Array:permute(FSharpFunc`2,ref):ref (7 methods)
       -1200 (-22.45% of base) : System.Private.CoreLib.dasm - UnmanagedMemoryAccessor:ReadArray(long,ref,int,int):int:this (6 methods)
       -1200 (-24.84% of base) : System.Private.CoreLib.dasm - UnmanagedMemoryAccessor:WriteArray(long,ref,int,int):this (6 methods)
       -1050 (-12.10% of base) : System.Collections.dasm - Enumerator:System.Collections.IEnumerator.get_Current():Object:this (56 methods)
        -998 (-42.41% of base) : System.Data.Common.dasm - DataColumnCollection:CanRemove(DataColumn,bool):bool:this
        -981 (-12.74% of base) : Newtonsoft.Json.dasm - <ExecuteFilter>d__4:MoveNext():bool:this (8 methods)
        -924 (-19.00% of base) : System.Private.CoreLib.dasm - SafeBuffer:ReadArray(long,ref,int,int):this (6 methods)
        -924 (-19.00% of base) : System.Private.CoreLib.dasm - SafeBuffer:WriteArray(long,ref,int,int):this (6 methods)
        -903 (-10.67% of base) : System.Collections.dasm - Enumerator:MoveNext():bool:this (56 methods)
        -897 (-41.84% of base) : System.Linq.Expressions.dasm - LabelInfo:ValidateJump(LabelScopeInfo):this (2 methods)
        -882 (-13.60% of base) : System.Linq.Queryable.dasm - Queryable:Average(IQueryable`1,Expression`1):double (21 methods)
Top method regressions (percentages):
          35 (350.00% of base) : System.Configuration.ConfigurationManager.dasm - ConfigurationSectionCollection:.ctor(SerializationInfo,StreamingContext):this
          35 (350.00% of base) : System.Management.dasm - ManagementNamedValueCollection:.ctor(SerializationInfo,StreamingContext):this
          35 (350.00% of base) : System.Net.Requests.dasm - FileWebResponse:.ctor(SerializationInfo,StreamingContext):this
          35 (350.00% of base) : System.Security.Cryptography.X509Certificates.dasm - X509Certificate2:.ctor(SerializationInfo,StreamingContext):this
          35 (250.00% of base) : System.Net.Requests.dasm - HttpWebResponse:.ctor(SerializationInfo,StreamingContext):this
          33 (31.73% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:GetQueryLambdaParameterNameRight(ImmutableArray`1):String
          12 (31.58% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - OverloadResolution:.ctor():this
           7 (26.92% of base) : System.Private.Xml.dasm - XmlSchemaSet:InternalValidationCallback(Object,ValidationEventArgs):this
           5 (26.32% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Conversions:.ctor():this (2 methods)
          22 (23.66% of base) : xunit.console.dasm - CommandLine:GuardNoOptionValue(KeyValuePair`2)
          20 (22.47% of base) : System.Data.Common.dasm - DataRow:CheckColumn(DataColumn):this
           8 (20.00% of base) : System.Private.DataContractSerialization.dasm - XmlDictionaryWriter:StartCanonicalization(Stream,bool,ref):this
           8 (20.00% of base) : System.Private.DataContractSerialization.dasm - XmlDictionaryWriter:EndCanonicalization():this
           8 (20.00% of base) : System.Private.DataContractSerialization.dasm - XmlDictionaryReader:StartCanonicalization(Stream,bool,ref):this
           8 (20.00% of base) : System.Private.DataContractSerialization.dasm - XmlDictionaryReader:EndCanonicalization():this
           8 (20.00% of base) : System.Private.DataContractSerialization.dasm - XmlDictionaryReader:ReadValueAsBase64(ref,int,int):int:this
           8 (20.00% of base) : System.Private.DataContractSerialization.dasm - XmlObjectSerializer:InternalWriteStartObject(XmlWriterDelegator,Object):this
           8 (20.00% of base) : System.Private.DataContractSerialization.dasm - XmlObjectSerializer:InternalWriteObjectContent(XmlWriterDelegator,Object):this
           8 (20.00% of base) : System.Private.DataContractSerialization.dasm - XmlObjectSerializer:InternalWriteEndObject(XmlWriterDelegator):this
           8 (20.00% of base) : System.Private.DataContractSerialization.dasm - XmlObjectSerializer:InternalIsStartObject(XmlReaderDelegator):bool:this
Top method improvements (percentages):
        -115 (-86.47% of base) : System.Data.Common.dasm - DataView:System.Collections.IList.set_Item(int,Object):this
        -115 (-86.47% of base) : System.Data.Common.dasm - DataView:System.Collections.IList.Clear():this
        -115 (-86.47% of base) : System.Data.Common.dasm - DataView:System.Collections.IList.Insert(int,Object):this
        -115 (-86.47% of base) : System.Data.Common.dasm - DataViewManager:System.Collections.IList.set_Item(int,Object):this
        -115 (-86.47% of base) : System.Data.Common.dasm - DataViewManager:System.Collections.IList.Add(Object):int:this
        -115 (-86.47% of base) : System.Data.Common.dasm - DataViewManager:System.Collections.IList.Clear():this
        -115 (-86.47% of base) : System.Data.Common.dasm - DataViewManager:System.Collections.IList.Insert(int,Object):this
        -115 (-86.47% of base) : System.Data.Common.dasm - DataViewManager:System.Collections.IList.Remove(Object):this
        -115 (-86.47% of base) : System.Data.Common.dasm - DataViewManager:System.Collections.IList.RemoveAt(int):this
        -115 (-86.47% of base) : System.Data.Common.dasm - LookupNode:Eval():Object:this
        -115 (-86.47% of base) : System.Data.Common.dasm - NameNode:Eval():Object:this
        -115 (-86.47% of base) : System.Data.OleDb.dasm - DbConnectionClosed:get_ServerVersion():String:this
        -115 (-86.47% of base) : System.Data.OleDb.dasm - DbConnectionClosed:Activate(Transaction):this
        -115 (-86.47% of base) : System.Data.OleDb.dasm - DbConnectionClosed:BeginTransaction(int):DbTransaction:this
        -115 (-86.47% of base) : System.Data.OleDb.dasm - DbConnectionClosed:Deactivate():this
        -115 (-86.47% of base) : System.Data.OleDb.dasm - DbConnectionClosed:EnlistTransaction(Transaction):this
        -115 (-86.47% of base) : System.Data.OleDb.dasm - DbConnectionClosed:GetSchema(DbConnectionFactory,DbConnectionPoolGroup,DbConnection,String,ref):DataTable:this
        -115 (-86.47% of base) : System.Data.OleDb.dasm - DbConnectionClosed:CreateReferenceCollection():DbReferenceCollection:this
         -88 (-83.02% of base) : System.Data.Common.dasm - DataAdapter:FillSchema(DataSet,int):ref:this
         -88 (-83.02% of base) : System.Data.Common.dasm - DataAdapter:Fill(DataSet):int:this
12274 total methods with Code Size differences (11855 improved, 419 regressed), 247173 unchanged.
Completed analysis in 35.55s

crossgen2 diff:

Total bytes of diff: -274844 (-0.73% of base)

As far as I understand, such blocks won't be even compiled in future (@AndyAyersMS 's #34522) but I guess it still makes sense to abort inlining inside them?

@dotnet/jit-contrib

category:cq
theme:inlining
skill-level:intermediate
cost:medium

[EgorBo]

Typical improvements:
EnumerableQuery.CreateQuery(...) such throwhelpers are used a lot in LINQ and System.Data

UnmanagedMemoryAccessor.ReadArray(...) SR.SomeStr is inlined into "key str" + SR.GetStringResource call
^ this method is also a top method in the #34522 jit-diff.

There are a few minor regressions, mainly because sometimes inlining decreases caller size. E.g. https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/DiagnosticUtility.cs#L47-L50

[AndyAyersMS]

I like your thinking here, but I would try and work within the current policy framework rather than override it.

• Always inlines should decrease code size and we should allow them.
• Discretionary inlines should be discouraged; we currently do this (we fix the multiplier to 1.3) but could go further;
• Force inlines should be honored.

Also there are times (though we're not smart enough to spot them yet) where we may want to inline more aggressively in a cold path to reveal facts that allow us to optimize a hotter path. So I'd prefer to keep this information as part of tunable policy and not as a fatal observation.

Can you try smaller multipliers for the InlineCallsiteFrequency::RARE case in DefaultPolicy::DetermineMultiplier? Perhaps a range of these to see just how sensitive things are to this number?

And FYI, I'm hoping to start chipping away at various sorts of profile support in the 6.0 timeframe so we have better guidance for the inliner.

[EgorBo]

Can you try smaller multipliers for the InlineCallsiteFrequency::RARE case in DefaultPolicy::DetermineMultiplier? Perhaps a range of these to see just how sensitive things are to this number?

These throwhelpers in LINQ, System.Data and in my sample (GetMyException) are bellow ALWAYS_INLINE size so the multiplier is not even calculated - InlineCallsiteFrequency::RARE is not used.

Successfully inlined Program:GetMyException():System.Exception:this (11 IL bytes) (depth 1) [below ALWAYS_INLINE size]

in my case these 11 bytes of IL lead to +46 bytes of asm in a cold block 🙂

[EgorBo]

Regarding InlineCallsiteFrequency::RARE I tried to change the multiplier (default: 1.3) to the following values:

0.1 - Total bytes of diff: 0 (0.00% of base)
0.5 - Total bytes of diff: 0 (0.00% of base)
1.35 - Total bytes of diff: 0 (0.00% of base)
1.4 - Total bytes of diff: -120 (-0.00% of base)
1.5 - Total bytes of diff: -63 (-0.00% of base)
2.0 - some big positive diff I didn't record.

[AndyAyersMS]

Thanks. So you are saying these size increases all come from always inline cases? Last I looked the overwhelming majority of these were size-decreasing.

I still think we're better of overall doing some inlining in cold regions; we just need to pick the right candidates. We still do some analysis of always inlines, in particular the IL scan, so one possible route here is to look for IL visible properties that distinguish size-decreasing always inlines from size-increasing inlines and use that to inform policy.

I did something like that a while back when working on ML-derived heuristics; one of the heuristics this derived was the SizePolicy -- this was pretty good at detecting inlines that would not increase size. So we might use the SizePolicy for cold regions of the method.

[EgorBo]

Thanks. So you are saying these size increases all come from always inline cases?

Especially this pattern:

public class Program
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static void Test(int x)
    {
        if (x % 2 != 0)
            throw new ArgumentException(SR.Argument_NotPOT);
    }
}

internal static class SR
{
    internal static string Argument_NotPOT => GetResourceString(nameof(Argument_NotPOT));

    [MethodImpl(MethodImplOptions.NoInlining)] // simulates resource lookup
    internal static string GetResourceString(string resourceKey) => resourceKey;
}

We use this SR.ResourceKey everywhere and everywhere we inline them as "string literal (key) + GetResourceString call" but the thing is - for throw blocks JIT doesn't materialize strings and emits a CORINFO_HELP_STRCNS call to do it lazily.

A possible strategy: ldstr tokens aren't cheap in cold blocks.

[EgorBo]

Moving this to Future, in theory, it should be eventually fixed by #43914