Allow to inline methods with delegate invocation (at least methods marked with AggressiveInlining)

[dmitriyse]

Method with delegate invocation problem:

using System;
using System.Runtime.CompilerServices;

public class C {
    private MyStruct[] _s = new MyStruct[1];
    
    public void M() {
        // Currently this method is not inlined :(
        _s[0].TryInvoke();
    }
}

public struct MyStruct
{
    public Action A;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void TryInvoke()
    {
        A();
        A();
    }
}

https://sharplab.io/#v2:C4LghgzgtgPgAgBgARwIwG4CwAoRLUB0ASgK4B2wAllAKYEDCA9lAA6UA2NATgMrcBulAMY0IWbDjgBmFACYk9JAG8cSNUhZdK/MMBpIAsgE8ewLiSHAA2gF0kAfQhIAvEjI0A7oZNmL11Dbi6kiq6tIoACyGABQAlMqhwWoA9MkKJFxcNBTsRkjAABaUTrSFjAAmSMVujMBVZOyU7pUg0YlJjlYINgQAKlxGAJJk/IwA1jRxQeoAvjhzErgyEL6W3qbmljgq2MHhaPIAguLtVgY0ZeWDrOzR55fXLOwA8ixUjGQQBIcA5j9ZEAg2how0aZCaP1iNna+yi/SGI3Gk1i7R2SXUhym7WCmNi0zUCwWQA==

category:cq
theme:inlining
skill-level:expert
cost:small

[AndyAyersMS]

@dmitriyse thanks -- would still love to see an example where allowing this sort of inlining has a signfiicant performance impact. Right now the costs are likely dominated by the delegate invokes, whether or not TryInvoke is inlined.

[dmitriyse]

I am working on prototype of "C# coroutines" where each virtual/non virtual call gives performance degradation, unfortunately work state is far from benchmarks.
But in Task based EventLoop implementation I was forced to maually inline this method (~ 5 times):

            [MethodImpl(MethodImplOptions.AggressiveInlining)]
            public void ExecuteEvent()
            {
                if (SimpleAction != null)
                {
                    SimpleAction();
                }
                else if (Action != null)
                {
                    // ReSharper disable once PossibleNullReferenceException
                    Action(State);
                }
                else
                {
                    Debug.Assert(Task != null, "Task != null");

                    // ReSharper disable once PossibleNullReferenceException
                    // ReSharper disable once AssignNullToNotNullAttribute
                    _scheduler.TryExecuteTaskInternal(Task);
                }
            }

This gives me performance speed up from 23 Mops/sec to 30 Mops/sec. (On one Core)

In a "C# coroutines" prototype I am targeting at least 100 Mops/sec. So this type of inlining is even more critical.

[mikedn]

at least 100 Mops/sec

That sounds a bit far fetched. What does an "operation" is supposed to consist of? With current CPUs it probably needs to consist of less than 100 instructions to reach such a speed. Even 100 is extremely optimistic...

[dmitriyse]

Motivations and some benchmarks can be found here:
dotnet/corefxlab#2168
https://github.com/dotnet/corefx/issues/28025

[omariom]

@AndyAyersMS @mikedn

I suspect this restriction is from pre-2.0 times. I remember in .NET Framework 2.0 release notes it was mentioned that delegate calls had got significantly faster.

Nowadays delegate calls can be like 50% faster than interface calls, yet interface call-sites don't prevent the containing methods from being inlined.

Removing the restriction will let JIT do a little bit less work. No?

[AndyAyersMS]

Could be. I have some work in progress to move expansion of delegate invoke earlier. As part of that I'll also look into what is behind this inlining block.

[AndyAyersMS]

Looks like we could remove this block, but broadly speaking it looks like all that happens is that we increase code size -- knock-on benefits seem to be rare. Part of this is that the inlinee still ends up with a call, one that we can't inline, and with a calling sequence is usually more complex than the call to the inlinee.

Delegate invoke is not all that frequent, so net regression is small, but there are virtually no code size improvements.

• CommandLine has a few delegate invoking methods (eg Trial.Either) marked as aggressive inline; these are now inlined.
• System.Management has calls to ManagementQuery:FireIdentifierChanged, these now expand to a conditinally executed delegate call, so perhaps some small perf win.
• I didn't see any cases in my sampling of diffs where we connected delegate creation with delegate invocation -- this is the one case we might eventually be able to optimize, via object stack allocation and various other fixes in the jit.

PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for x64 default jit
Summary of Code Size diffs:
(Lower is better)
Total bytes of diff: 30821 (0.06% of base)
    diff is a regression.
Top file regressions (bytes):
       13367 : CommandLine.dasm (3.01% of base)
        3334 : Microsoft.CodeAnalysis.dasm (0.19% of base)
        2552 : System.Private.Xml.dasm (0.07% of base)
        2157 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (0.07% of base)
        2144 : System.Management.dasm (0.55% of base)
        1275 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.02% of base)
        1138 : System.Net.Sockets.dasm (0.58% of base)
         634 : System.Threading.Tasks.Dataflow.dasm (0.08% of base)
         530 : Newtonsoft.Json.dasm (0.06% of base)
         484 : Microsoft.CodeAnalysis.CSharp.dasm (0.01% of base)
         465 : System.Data.Common.dasm (0.03% of base)
         336 : xunit.execution.dotnet.dasm (0.14% of base)
         295 : Microsoft.Extensions.Primitives.dasm (1.12% of base)
         233 : System.Runtime.Serialization.Formatters.dasm (0.24% of base)
         215 : System.Security.Cryptography.Primitives.dasm (0.62% of base)
         200 : System.Private.DataContractSerialization.dasm (0.03% of base)
         193 : Microsoft.Extensions.Http.dasm (0.44% of base)
         148 : System.Private.CoreLib.dasm (0.00% of base)
         148 : xunit.runner.utility.netcoreapp10.dasm (0.08% of base)
         116 : xunit.console.dasm (0.13% of base)
37 total files with Code Size differences (0 improved, 37 regressed), 227 unchanged.
Top method regressions (bytes):
        2742 (78.25% of base) : CommandLine.dasm - Trial:Collect(IEnumerable`1):Result`2 (7 methods)
        2742 (95.54% of base) : CommandLine.dasm - ResultExtensions:Collect(IEnumerable`1):Result`2 (7 methods)
        2670 (49.74% of base) : CommandLine.dasm - ResultExtensions:Flatten(Result`2):Result`2 (7 methods)
         643 ( 7.18% of base) : CommandLine.dasm - HelpText:AutoBuild(ParserResult`1,Func`2,Func`2,bool,int):HelpText (7 methods)
         540 (22.21% of base) : CommandLine.dasm - Trial:Flatten(Result`2):Result`2 (7 methods)
         531 (42.28% of base) : CommandLine.dasm - Trial:MergeMessages(IEnumerable`1,Result`2):Result`2 (7 methods)
         478 (62.00% of base) : CommandLine.dasm - Tokenizer:PreprocessDashDash(IEnumerable`1,Func`2):Result`2
         422 (23.25% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:ParseDocumentContentAsync():Task`1:this
         402 (23.50% of base) : Microsoft.CodeAnalysis.dasm - <DescendantTriviaIntoTrivia>d__161:MoveNext():bool:this
         369 (14.65% of base) : Microsoft.CodeAnalysis.dasm - <DescendantNodesAndTokensIntoTrivia>d__159:MoveNext():bool:this
         340 (14.26% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - TraceLog:FastSerialization.IFastSerializable.ToStream(Serializer):this
         333 (19.69% of base) : System.Private.Xml.dasm - XmlSchemaValidator:ElementValidationError(XmlQualifiedName,ValidationState,ValidationEventHandler,Object,String,int,int,XmlSchemaSet)
         320 (96.68% of base) : Microsoft.CodeAnalysis.dasm - CryptographicHashProvider:TryGetAlgorithm(int):HashAlgorithm (2 methods)
         295 (40.80% of base) : Microsoft.CodeAnalysis.dasm - AbstractSyntaxNavigator:GetPreviousToken(SyntaxTrivia,SyntaxTriviaList,Func`2,Func`2,byref):SyntaxToken:this
         294 (25.61% of base) : CommandLine.dasm - <>c__DisplayClass17_0`1:<DisplayHelp>b__0(IEnumerable`1):this (7 methods)
         275 (51.89% of base) : Microsoft.CodeAnalysis.dasm - AbstractSyntaxNavigator:GetLastToken(SyntaxTriviaList,Func`2,Func`2):SyntaxToken:this
         273 ( 2.55% of base) : CommandLine.dasm - HelpText:AutoBuild(ParserResult`1,int):HelpText (7 methods)
         244 (17.63% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:ParseElementContentAsync():Task`1:this
         228 (11.57% of base) : System.Private.Xml.dasm - XmlWellFormedWriter:AdvanceStateAsync(int):Task:this
         220 (15.88% of base) : Microsoft.CodeAnalysis.dasm - <DescendantTriviaOnly>d__160:MoveNext():bool:this
Top method improvements (bytes):
         -21 (-0.83% of base) : System.Net.Http.WinHttpHandler.dasm - HttpKnownHeaderNames:TryGetHeaderName(__Canon,int,int,Func`3,Func`5,byref):bool
          -4 (-0.22% of base) : CommandLine.dasm - <>c__DisplayClass0_2`1:<Build>b__15():__Canon:this
          -2 (-0.41% of base) : xunit.performance.execution.dasm - BenchmarkEventSource:BenchmarkIterationStop(String,String,int,long):this
Top method regressions (percentages):
          64 (914.29% of base) : System.Private.CoreLib.dasm - _ThreadPoolWaitOrTimerCallback:WaitOrTimerCallback_Context_f(Object)
          64 (640.00% of base) : System.Private.CoreLib.dasm - _ThreadPoolWaitOrTimerCallback:WaitOrTimerCallback_Context_t(Object)
          31 (620.00% of base) : System.Management.dasm - ManagementOptions:HandleIdentifierChange(Object,IdentifierChangedEventArgs):this
          31 (387.50% of base) : System.Management.dasm - EventWatcherOptions:set_BlockSize(int):this
          52 (288.89% of base) : System.Runtime.Serialization.Formatters.dasm - SerializationEvents:InvokeOnSerializing(Object,StreamingContext):this
          52 (288.89% of base) : System.Runtime.Serialization.Formatters.dasm - SerializationEvents:InvokeOnDeserializing(Object,StreamingContext):this
          52 (288.89% of base) : System.Runtime.Serialization.Formatters.dasm - SerializationEvents:InvokeOnDeserialized(Object,StreamingContext):this
          31 (281.82% of base) : System.Management.dasm - ManagementScope:HandleIdentifierChange(Object,IdentifierChangedEventArgs):this
          43 (215.00% of base) : Microsoft.CSharp.dasm - UnsafeMethods:IUnknownReleaseNotZero(long)
          30 (214.29% of base) : System.Management.dasm - ConnectionOptions:set_Impersonation(int):this
          30 (214.29% of base) : System.Management.dasm - ConnectionOptions:set_Authentication(int):this
          30 (136.36% of base) : System.Management.dasm - ConnectionOptions:set_EnablePrivileges(bool):this
          75 (131.58% of base) : System.Net.Sockets.dasm - SocketPal:Disconnect(Socket,SafeSocketHandle,bool):int
         116 (116.00% of base) : CommandLine.dasm - MaybeExtensions:Map(Maybe`1,Func`2):Maybe`1 (7 methods)
         116 (116.00% of base) : CommandLine.dasm - MaybeExtensions:Select(Maybe`1,Func`2):Maybe`1 (7 methods)
         320 (96.68% of base) : Microsoft.CodeAnalysis.dasm - CryptographicHashProvider:TryGetAlgorithm(int):HashAlgorithm (2 methods)
        2742 (95.54% of base) : CommandLine.dasm - ResultExtensions:Collect(IEnumerable`1):Result`2 (7 methods)
          36 (94.74% of base) : System.Management.dasm - ObjectGetOptions:set_UseAmendedQualifiers(bool):this
          90 (90.00% of base) : CommandLine.dasm - MaybeExtensions:Bind(Maybe`1,Func`2):Maybe`1 (7 methods)
          26 (89.66% of base) : System.Management.dasm - ManagementNamedValueCollection:Remove(String):this
Top method improvements (percentages):
         -21 (-0.83% of base) : System.Net.Http.WinHttpHandler.dasm - HttpKnownHeaderNames:TryGetHeaderName(__Canon,int,int,Func`3,Func`5,byref):bool
          -2 (-0.41% of base) : xunit.performance.execution.dasm - BenchmarkEventSource:BenchmarkIterationStop(String,String,int,long):this
          -4 (-0.22% of base) : CommandLine.dasm - <>c__DisplayClass0_2`1:<Build>b__15():__Canon:this
448 total methods with Code Size differences (3 improved, 445 regressed), 243342 unchanged.

So will likely hold off on this until we find more compelling motivation.

Prototype changes here: master...AndyAyersMS:AllowInlineeDelegateInvoke

[nietras]

There are many reasons I could think of doing this, just check sorting code, which has to duplicate code paths to get optimal code generation for different scenarios. This is a general problem in .NET due to the canonical representation of code for reference types, so typically you might have to have different code paths for value types vs. reference types. The injection of a value type as a functor that can be fully inlined if needed (e.g. for value types) is a great pattern for this (which does not necessarily lead to overall increased code size, but instead much more relevant reduced source code size. The overhead for sorting is very much measurable.

So in my view there is compelling motivation :)

[stephentoub]

just check sorting code, which has to duplicate code paths to get optimal code generation for different scenarios

What duplicate sorting code would be removable if methods containing delegate invocations were inlined?

[nietras]

What duplicate sorting code would be removable if methods containing delegate invocations were inlined?

@stephentoub well this is all subject to interpretation, but the current sorting code is not optimal for a some code paths. Which I have touched upon before. Currently, there are three code paths Comparison<T>, T where T : IComparable<T>, TComparer where TComparer : IComparer<T> depending on keys or keys and values and some other stuff. TComparer isn't implemented as the original API proposal wanted it (that is me 😁). None of these three code paths are actually optimal and on par with old sorting code for some primitives, that is the C++ version, which did not rely on IComparable<T>.

I have been (occasionally) trying to see if these four versions could be consolidated to one single source code version, that is still optimal for all scenarios. Outside the context of the dotnet runtime, since it seemed to me this was not a priority. Using hacks, trickery and other probably frowned upon changes I believe there is way... having the below call be inlined, would "solve" one part of the problem.

        readonly struct OpenDelegateObjectComparer : IComparer<object>
        {
            readonly Comparison<object> m_compare;

            public OpenDelegateObjectComparer(Comparison<object> compare) =>
                m_compare = compare;

            [MethodImpl(MethodImplOptions.AggressiveInlining)]
            public int Compare(object x, object y) => m_compare(x, y);
        }

The impact here compared to calling the comparison is significant going from 0.87 (Comparison<object>) to 1.03 (OpenDelegateObjectComparer) in a benchmark.

I understand that this is quite esoteric need, but in general having great support for struct functors that are actually inlined is very much appreciated for low level code. This works mostly for value types. For reference types or when there is a delegate things fall apart and you have to duplicate code. Despite code being quite "generic".