Runtime: Get InvocationList of MulticastDelegate without any allocation

[RamType0]

Background and Motivation

Currently,we have an API to invocation list of MulticastDelegate,it is MulticastDelegate.GetInvocationList().

It allocates array every time we call it.
And maybe that's why we have internal HasSingleTarget property in it.

We need handsome,and performant API.

Proposed API

```C#
public ReadOnlySpan InvocationList => delegates == null ? MemoryMarshal.CreateSpan(this,1) : delegates;

```

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[GrabYourPitchforks]

We need handsome,and performant API.

This doesn't really describe the motivation, just the desired solution. Where do you see this API being used? Do you anticipate those scenarios seeing a measurable perf benefit? That would help justify the work needed for this API.

Thanks!

[RamType0]

I have used this API to handle exception from each invoked delegate.

[GrabYourPitchforks]

Would syntax like the following work for you instead? I don't think there's a reliable way to create a one-element ROS<Delegate> without allocating or introducing a new field to MulticastDelegate, neither of which would be desirable.

SomeDelegate myDelegate = GetDelegate();
foreach (Delegate innerDelegate in myDelegate)
{
    var castDelegate = (SomeDelegate)innerDelegate;
    // use 'castDelegate' here
}

You should also spend some time prototyping this to make sure that the performance is what you expect. In your own application, make sure that the cost of this allocation really is measurable. You can use FieldInfo.GetValue via reflection to access the private _invocationList instance field for the purpose of testing.

[RamType0]

Ah,I just mistaken that we need ref Delegate for CreateSpan,but not Delegate.
But there are still available solution.
alter type of MuticastDelegate.delegates to object,and just assign it self to it if it is not multicasting.
Then, implementation would become
C# public ReadOnlySpan<Delegate> InvocationList => delegates == this ? MemoryMarshal.CreateSpan<Delegate>(ref delegates,1) : Unsafe.As<Delegate[]>( delegates);

[GrabYourPitchforks]

just assign it self to it if it is not multicasting.

That's probably not a viable solution. There's a lot of code in the runtime - both managed and unmanaged - that has knowledge about the exact layout of delegate instances and what all the different instance fields represent. It would be very risky (and possibly also a negative perf hit) to update all of those call sites. That's not a good tradeoff for a niche API.

[RamType0]

Aren't you confusing it with field of Delegate?
I saw many comment which is similar to what you say in Delegate.
But I have not seen such a comment in MulticastDelegate.

[GrabYourPitchforks]

But I have not seen such a comment in MulticastDelegate.

See here for some examples. You can follow the call graphs backward and see that there's a decent amount of code which relies on this field containing a restricted set of possible values.

[RamType0]

Oh,maybe I was seeing mono's one.

[bollhals]

SomeDelegate myDelegate = GetDelegate();
foreach (Delegate innerDelegate in myDelegate)
{
    var castDelegate = (SomeDelegate)innerDelegate;
    // use 'castDelegate' here
}

Such syntax (as long as it is not allocating) would actually be very nice for when you'd have to invoke each delegate within a try catch block to ensure each gets called. So e.g.

SomeDelegate myDelegate = GetDelegate();
foreach (Delegate innerDelegate in myDelegate)
{
    try
    {
        // invoke single delegate target
        ((SomeDelegate)innerDelegate).Invoke()
    }
    catch (Exception exception)
    {
        //Do something with that exception
    }
}

As of right now we (Example) just call GetInvocationList() which always allocates. (It shows up already in measurements, although not yet high up in the list. Still working on reducing others first)

[RamType0]

I have just created benchmark project.
In this project,we have these variants of GetInvocationlist.

• CoreCLRGetInvocationList
  Just a copy from current CoreCLR's implementation.

• MyGetInvocationList
  My optimized implementation of GetInvocationList without altering or adding any API.

• UnsafeGetInvocationList
  My optimized allocation less new API.
  This is unsafe because the result will become wrong if we access the result after altering the delegate.
  But if we use this correctly, it is very fast and safe.

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.508 (2004/?/20H1)
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=5.0.100
  [Host]     : .NET Core 5.0.0 (CoreCLR 5.0.20.51904, CoreFX 5.0.20.51904), X64 RyuJIT
  DefaultJob : .NET Core 5.0.0 (CoreCLR 5.0.20.51904, CoreFX 5.0.20.51904), X64 RyuJIT

| Method | N | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------- |----- |-------------:|------------:|-----------:|-------:|------:|------:|----------:|
| CoreCLRGetInvocationList | 1 | 10.774 ns | 0.1214 ns | 0.1077 ns | 0.0038 | - | - | 32 B |
| MyGetInvocationList | 1 | 9.302 ns | 0.1656 ns | 0.1549 ns | 0.0038 | - | - | 32 B |
| UnsafeGetInvocationList | 1 | 2.917 ns | 0.0370 ns | 0.0346 ns | - | - | - | - |
| CoreCLRGetInvocationList | 10 | 82.291 ns | 0.7804 ns | 0.7299 ns | 0.0124 | - | - | 104 B |
| MyGetInvocationList | 10 | 23.457 ns | 0.4810 ns | 0.4499 ns | 0.0124 | - | - | 104 B |
| UnsafeGetInvocationList | 10 | 7.849 ns | 0.0776 ns | 0.0726 ns | - | - | - | - |
| CoreCLRGetInvocationList | 100 | 732.493 ns | 10.2674 ns | 9.6041 ns | 0.0982 | - | - | 824 B |
| MyGetInvocationList | 100 | 154.543 ns | 3.0153 ns | 4.2270 ns | 0.0985 | - | - | 824 B |
| UnsafeGetInvocationList | 100 | 53.075 ns | 0.5351 ns | 0.4743 ns | - | - | - | - |
| CoreCLRGetInvocationList | 1000 | 7,922.301 ns | 100.5212 ns | 89.1095 ns | 0.9460 | - | - | 8024 B |
| MyGetInvocationList | 1000 | 1,405.915 ns | 9.4829 ns | 8.8703 ns | 0.9575 | - | - | 8024 B |
| UnsafeGetInvocationList | 1000 | 676.750 ns | 6.3342 ns | 5.6151 ns | - | - | - | - |

[GrabYourPitchforks]

@RamType0 The benchmark code makes incorrect assumptions about the layout of that field, and it makes incorrect assumptions about how the C# language treats reachability of objects passed as _in_ parameters. This could lead to reliability problems at runtime.

I'll re-up my comment from https://github.com/dotnet/runtime/issues/41849#issuecomment-686898306. We could probably add an enumerator to allow this scenario if it's really that important, but creating a ROS<T> is likely a non-starter. For now, a workaround could be to call GetInvocationList once and cache the result.

[RamType0]

@RamType0 The benchmark code makes incorrect assumptions about the layout of that field, and it makes incorrect assumptions about how the C# language treats reachability of objects passed as _in_ parameters. This could lead to reliability problems at runtime.

This caused just by MemoryMarshal.CreateReadOnlySpan.
Actually, C# compiler correctly handles this.
C# public static ref readonly T UnsafeGetInvocationListByRefAndCount<T>(in T d,out int count) where T : MyMulticastDelegate { var _invocationList = d._invocationList; if (_invocationList != null) { var invocationList = Unsafe.As<object[]>(_invocationList); count = (int)d._invocationCount; return ref Unsafe.As<object, T>(ref MemoryMarshal.GetArrayDataReference(invocationList)); } else { count = 1; return ref d; } }
So, it is not a UnsafeGetInvocationList specific problem.
And also, this problem has been forgiven for MemoryMarshal.CreateReadOnlySpan, which is already approved public API.

I'll re-up my comment from #41849 (comment). We could probably add an enumerator to allow this scenario if it's really that important, but creating a ROS<T> is likely a non-starter. For now, a workaround could be to call GetInvocationList once and cache the result.

It is better than current API.
But we have thought that List.Enumerator is slow, and we have finally created CollectionsMarshal.AsSpan.
So I proposed API for creating ReadOnlySpan.

[bollhals]

I'll re-up my comment from #41849 (comment). We could probably add an enumerator to allow this scenario if it's really that important, but creating a ROS<T> is likely a non-starter. For now, a workaround could be to call GetInvocationList once and cache the result.

In which use case (where you'd use this API) would a ROS actually be more beneficial than an enumerator?

@GrabYourPitchforks In your example you've set the type of the loop variable to Delegate, I assume it wouldn't be possible and/or safe to type that to the actual delegate type (e.g. foreach (Action<int> innerDelegate in myDelegate)?

[GrabYourPitchforks]

@bollhals If it's an instance method, the type would be Delegate, and the caller would need to cast. (These casts should be very cheap.) If it's an extension method, it can probably be typed appropriately.

[Joe4evr]

But we have thought that List.Enumerator is slow

Exactly what is slow about it?

[RamType0]

But we have thought that List.Enumerator is slow

Exactly what is slow about it?

using foreach for List<T> is slower than using foreach for T[].
Then, we have finally created CollectionsMarshal.AsSpan.
This unsafe API was created just for performance.

[RamType0]

@GrabYourPitchforks BTW, MyGetInvocationList is seems to be better than CoreCLRGetInvocationList without any API changes.
If I created PR for it, is it beneficial?

[BreyerW]

If im not mistaken real reason was performance of list with large structs. Eg imagine a list with million Matrix4x4 structs. Each such matrix holds 16 floats making it pretty bulky and each such struct had to be copied every time a value was retrieved via foreach in our case meaning potentially 1 milion copies of relatively large struct. This can definitely tank performance. Class based cases are largely unaffected unless u wanted to swap references itself during iteration

[RamType0]

If im not mistaken real reason was performance of list with large structs. Eg imagine a list with million Matrix4x4 structs. Each such matrix holds 16 floats making it pretty bulky and each such struct had to be copied every time a value was retrieved via foreach in our case meaning potentially 1 milion copies of relatively large struct. This can definitely tank performance. Class based cases are largely unaffected unless u wanted to swap references itself during iteration

At least, the posted benchmark uses int for element, which is smaller than "reference".

[Joe4evr]

using foreach for List<T> is slower than using foreach for T[].

Being skeptical of a post made more than 10 years ago, I went and did a proper benchmark.

---

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.572 (2004/?/20H1)
Intel Core i7-10610U CPU 1.80GHz, 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.100
  [Host]        : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
  .NET Core 3.1 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
  .NET Core 5.0 : .NET Core 5.0.0 (CoreCLR 5.0.20.51904, CoreFX 5.0.20.51904), X64 RyuJIT

| Method | Job | Runtime | Mean | Error | StdDev | Median |
|----------------- |-------------- |-------------- |----------:|----------:|----------:|----------:|
| LargeStructList | .NET Core 3.1 | .NET Core 3.1 | 78.698 ns | 0.8484 ns | 0.7084 ns | 78.545 ns |
| LargeStructArray | .NET Core 3.1 | .NET Core 3.1 | 28.893 ns | 0.2721 ns | 0.2546 ns | 28.873 ns |
| ClassList | .NET Core 3.1 | .NET Core 3.1 | 73.004 ns | 1.4792 ns | 3.0217 ns | 74.361 ns |
| ClassArray | .NET Core 3.1 | .NET Core 3.1 | 8.114 ns | 0.0707 ns | 0.0661 ns | 8.104 ns |
| IntList | .NET Core 3.1 | .NET Core 3.1 | 42.759 ns | 0.2340 ns | 0.1954 ns | 42.711 ns |
| IntArray | .NET Core 3.1 | .NET Core 3.1 | 7.497 ns | 0.1566 ns | 0.1465 ns | 7.479 ns |
| LargeStructList | .NET Core 5.0 | .NET Core 5.0 | 70.169 ns | 0.9476 ns | 0.8400 ns | 69.955 ns |
| LargeStructArray | .NET Core 5.0 | .NET Core 5.0 | 24.355 ns | 0.5135 ns | 0.9893 ns | 24.142 ns |
| ClassList | .NET Core 5.0 | .NET Core 5.0 | 67.844 ns | 0.6781 ns | 0.6343 ns | 67.873 ns |
| ClassArray | .NET Core 5.0 | .NET Core 5.0 | 6.542 ns | 0.0661 ns | 0.0586 ns | 6.543 ns |
| IntList | .NET Core 5.0 | .NET Core 5.0 | 44.817 ns | 0.7760 ns | 0.7258 ns | 44.625 ns |

| IntArray | .NET Core 5.0 | .NET Core 5.0 | 7.864 ns | 0.2957 ns | 0.8718 ns | 7.717 ns |

So I concede that lists are slower than a raw array, however I also don't think this difference is the end of the world, provided you're not literally on fire on a burning hot path. Now I'm very curious on where this difference comes from, since I was assuming it could get much closer to array performance than this.

[stephentoub]

Now I'm very curious on where this difference comes from

foreach on an array is desugared by the C# compiler into simple for loop. foreach on a list needs to go through its struct Enumerator, does version checks to make sure the list hasn't changed between iterations, etc.

e.g.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8lAbhpuIGYnSGBhBgG8aDMUx4BLAHYYGAQShRsATwAUMjAG0Aug0kYY+XAEpR4kdXHX9shrgCu+BgF4GddlZtiAZtBjYYAAWDBp2krb6hsYm9k4MANRukp7eYsQA7HH4qeIAvuZihRK2cgAykrgYahVVADyaAHxRRqbFlmml2a7uud5+sIEhYXIRMi0x3Un6fTaZ2bMF1HlAA===

[RamType0]

@Joe4evr
Thanks for posting the benchmark! I couldn't find benchmark in recent environment.

So I concede that lists are slower than a raw array, however I also don't think this difference is the end of the world, provided you're not literally on fire on a burning hot path. Now I'm very curious on where this difference comes from, since I was assuming it could get much closer to array performance than this.

"foreach" for span or array is converted to "for".
And also, the generated pattern of "for" eliminates bounds check to throwing IndexOutOfRangeException.
Both of these optimization is specific for array or span.

It is also seems that IntList is significant faster than ClassList.
Maybe this is because of array's covariant check.

[GrabYourPitchforks]

The CollectionsMarshal class was added to allow efficient bulk operations over the list, such as bulk moving data or bulk modification of data. It wasn't added because of any perceived deficiency in List's enumerator.

We're getting very off-track here. If the request is specifically for projection of a MulticastDelegate as a ROS over its inner targets, then that is not feasible (as previously mentioned) and this issue should be closed. This issue provided several alternative designs, but if we don't have a consumer for those alternative APIs then this work will never rise above the cut line for any release.

[bollhals]

We're getting very off-track here. If the request is specifically for projection of a MulticastDelegate as a ROS over its inner targets, then that is not feasible (as previously mentioned) and this issue should be closed. This issue provided several alternative designs, but if we don't have a consumer for those alternative APIs then this work will never rise above the cut line for any release.

For me I think the main goal should be that there is a simple and easily accessable way (that has zero allocation) to iterate over the individual delegates. Ideally this improves performance but this should not be performance critical.