Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the IAction***, IFunc*** abstractions and make C# automatically inherit them by delegates #3336

Closed
dmitriyse opened this issue Apr 4, 2020 · 11 comments

Comments

@dmitriyse
Copy link

dmitriyse commented Apr 4, 2020

Instantiated delegates are not interchangeable even if they have compatible signatures.
In performance-critical methods this causes small performance leaks:

using System;
using System.Threading;

namespace TheProblem
{
    public static class MyLib
    {
           public static void ScheduleSomeWork(WaitCallback callback, object state) { ... }
    }
    public class Foo
    {
        public void RunOnMyThread(Action<object> action, object obj)
        {
            // error CS1503: Argument 1: cannot convert from 'System.Action<object>' to 'System.Threading.WaitCallback'
            // ThreadPool.QueueUserWorkItem(action, obj);

            // To avoid the error we needs to make a delegate conversion:
            // PERFORMANCE CRITICAL METHOD.
            MyLib.ScheduleSomeWork(
                state => ((Action)state)(), // Optimized to a singleton 
                (Action)(() => action(obj))); // Creates new delegate instance on each call 
            //(PERFORMANCE LEAK !)
        }
    }
}

To avoid this, C# languages and the Base Class Library could be improved (Action and Func delegates):

namespace Improvement
{
    // Introducing new family of contracts IAction***, IFunc***
    public interface IAction<in T1>
    {
        void Invoke(T1 arg1);
    }

    // Improving C# to automatically inherit appropriate contract from the IAction***, IFunc*** family,
    // Generated by the compiler from the "delegate void Action<in T1>(T1 arg1)"
    public class Action<T1> : /*Delegate,*/ IAction<T1> 
    {
        public void Invoke(T1 arg1)
        {
            // Compiled as IL instruction to call the target
        }
    }

    // Improving C# to automatically inherit appropriate contract from the IAction***, IFunc*** family,
    // Generated by the compiler from the "delegate void WaitCallback(object state);"
    public class WaitCallback : /*Delegate,*/ IAction<object>
    {
        public void Invoke(object arg1)
        {
            // Compiled as IL instruction to call the target
        }
    }

    public static class MyLib
    {
        // Adding new signatures to the BCL
        public static void ScheduleSomeWork<TWaitCallback>(TWaitCallback callback, object state)
            where TWaitCallback: IAction<object>
        {
            // Implementation
            
            callback.Invoke(state); // Transformed to IL instruction

            // Implementation
        }
    }

    public class Foo
    {
        public void RunOnMyThread(Action<object> action, object obj)
        {
            // After this improvement, we can directly pass any delegate with the Action<object> signature.
            // No Performance leak!
            MyLib.ScheduleSomeWork(action, obj);
        }
    }
}

Additional benefits of the proposed feature:

    // Not only delegates can benefits from IAction*** , IFunc*** contracts family.
    public class MyWorkItem : IAction<object>
    {
        public void Invoke(object arg1)
        {
            // Do my work
        }
    }

    public class Bar
    {
        public void RunOnMyThread(MyWorkItem workItem)
        {
            // No conversion required!
            MyLib.ScheduleSomeWork(workItem, null);

            // Previously it wasn't possible:
            // Argument 1: cannot convert from 'Improvement.MyWorkItem' to 'System.Threading.WaitCallback'
            //ThreadPool.QueueUserWorkItem(workItem, null);
        }
    }
@HaloFour
Copy link
Contributor

HaloFour commented Apr 5, 2020

This is not a language feature request as it would be the BCL's responsibility to add the interfaces and the runtime's responsibility to have delegate "automatically" implement them. I can't see this happening, though, as this would require that the BCL add thousands of interface types to account for all of the different permutations of arity and directionality that are possible.

@dmitriyse
Copy link
Author

@HaloFour thank you for the tip to move this proposal to the runtime.
I think in this repo we need to identify could be the same benefits be achieved with the #110 or not.
About permutations of all in/ref/out options:
Amount of permutations ~= 3 + 3^2+ 3^3 + ... + 3^(Nmax+1) where Nmax is the maximum number of parameters.
This is the table with numbers:

Nmax Permutations
1 12
2 39
3 120
4 363
5 1092
6 3279
7 9840
8 29523
9 88572
10 265719
11 797160
12 2391483
13 7174452
15 50221173
16 179361336

So if we will support this only for Nmax=8 it will be not so expensive.
(It's not so difficult to generate the assembly with all delegates/interfaces for any Nmax value and measure its size)

We already have proposals to bring such permutations for already existing Action, Func

For example in this proposal.
dotnet/roslyn#16488

@HaloFour
Copy link
Contributor

HaloFour commented Apr 5, 2020

@dmitriyse

So if we will support this only for Nmax=8 it will be not so expensive.

28k types would add an enormous amount of bloat to the runtime. The number of types loaded into a basic .NET Core 3.x console app today is less than 10% of that. Foisting that requirement on all .NET processes doesn't sound remotely reasonable.

@theunrepentantgeek
Copy link

Adding 28k additional types would almost certainly be a runtime breaking change for at least one of the supported target runtimes.

For a prior example of exactly this, see 65535 interfaces ought to be enough for anybody.

Also ... the overhead in moving work to another thread is significant. The exact amount depends on your processor architecture, memory architecture, operating system, current CPU load, and so on. If memory serves, "typical" would be in the range 100 microseconds to 50 milliseconds. A clock cycle or four to bounce execution through a nested delegate is probably so minor a different that you wouldn't be able to detect the difference for the statistical noise.

@YairHalberstadt
Copy link
Contributor

Also interface calls are slower than delegates, so I'm not even sure this would be a performance win.

@JustNrik
Copy link

JustNrik commented Apr 5, 2020

We don't need interfaces, we need implicit conversion between delegates with same signature. Also Unsafe.As is a thing btw. Unsafe.As<WaitCallback>(yourActionOfObject) should work

@jnm2
Copy link
Contributor

jnm2 commented Apr 6, 2020

@dmitriyse This is the pattern I've been using (usually a private static method rather than a lambda just to keep stack traces nice):

public void RunOnMyThread(Action<object> action, object obj)
{
    ThreadPool.QueueUserWorkItem(
        state =>
        {
            var (action, obj) = ((Action<object>, object))state!;
            action(obj);
        },
        state: (action, obj));
}

A lot of BCL code uses this technique but with Tuple instead of ValueTuple.

@jnm2
Copy link
Contributor

jnm2 commented Apr 6, 2020

Oh, look at this! .NET Core has a generic overload that allows you to even skip allocating for object state:

ThreadPool.QueueUserWorkItem(
    state => state.action(state.obj),
    state: (action, obj),
    preferLocal: true/false);

@dmitriyse
Copy link
Author

@theunrepentantgeek, @HaloFour thank you for the tip, that ~28K is an unacceptable increase of types count for the current runtime.

I have updated the topic.

I know about ThreadPool.QueueUserWorkItem and ThreadPool.UnsafeQueueUserWorkItem<T> and this is a very good example where the BCL library authors decided that the "delegate conversion and re-capturing" worth to be optimized.

For other library writers with a performance-critical part is also beneficial to have some "100% free" delegate conversion.

This proposal is NOT about improvements in BCL library code like QueueUserWorkItem.
This proposal about the feature that solves "a Delegate conversion costs" problem, even if it's small, QueueUserWorkItem proves that sometimes it worth to be optimized.

Yes this proposal is a duplicate of dotnet/runtime#4331
But I haven't found the implementation suggestion in this proposal.

Even if C# and runtime will allow to convert Action to WaitCallback silently,
the code like this will be broken:

       object myUnknown = ....  <- Action<Object>;
       if (myUnknown is WaitCallback tst) // Ok we hacked runtime to make "is operator" know about signature equivalence
       {  
       }
      
       if (myUnknown.Type.Name == "WaitCallback") // This code will be broken 
       {
              // Bad written code
       }

@dmitriyse
Copy link
Author

Alternatives:

@dmitriyse
Copy link
Author

Found a better approach, see dotnet/runtime#4331 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants