8.0 Discussion about public events

[bollhals]

Originating from #970 the discussion is about how we deal with the currently public events.

Starter was the post from @stebet

I'm wondering if a better implementation than having the events is to have a "handler" interface that can be registered in the channels, so instead of events like FlowControlChanged etc. there would be an interface called IChannelHandler, that could be registered like consumers. These handlers would exposes the standard methods:

public interface IChannelHandler
{
  ValueTask OnFlowControl(IChannel channel, bool active);
  ValueTask OnPublicAckReceived(IChannel channel);
  ...
}

That way it's easy to override whatever logic to take care of in a class (logging, waiting for publish confirms etc.) and then register it with the channel interface channel.RegisterHandler(myCustomHandler);.

The channel can maintain those handlers in a list and execute them (in parallel if needed) as events happen. That way those implementing the handlers can also do things asynchronously with minimal effort and the channel code would be free of all the async event workarounds we currently have. Logging and all sorts of instrumentation or special logic can then be added to augment the channel interface, especially if the handlers can have a reference to the Channel objects themselves and access other things.

I've done some measurements today regarding performance of various ways of invoking actions To have also an idea about what this means.

Method	action	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
GetInvocationList	Syste(...)nt32] [29]	14.7064 ns	1.0487 ns	0.0575 ns	1.00	0.00	0.0068	-	-	32 B
GetInvocationList	Syste(...)nt32] [29]	31.4116 ns	12.2659 ns	0.6723 ns	2.14	0.05	0.0085	-	-	40 B
Direct	Syste(...)nt32] [29]	0.6689 ns	0.1508 ns	0.0083 ns	0.05	0.00	-	-	-	-
Direct	Syste(...)nt32] [29]	7.2192 ns	0.2554 ns	0.0140 ns	0.49	0.00	-	-	-	-
Reflection.Emit	Syste(...)nt32] [29]	14.8313 ns	0.2667 ns	0.0146 ns	1.01	0.00	-	-	-	-
Reflection.Emit	Syste(...)nt32] [29]	25.1454 ns	0.8545 ns	0.0468 ns	1.71	0.01	-	-	-	-
Interface	Syste(...)Test] [66]	6.7287 ns	0.8310 ns	0.0455 ns	?	?	-	-	-	-
Interface	Syste(...)Test] [66]	11.7185 ns	1.1444 ns	0.0627 ns	?	?	-	-	-	-
List_Action	Syste(...)t32]] [64]	6.7017 ns	0.1550 ns	0.0085 ns	?	?	-	-	-	-
List_Action	Syste(...)t32]] [64]	11.5592 ns	0.1531 ns	0.0084 ns	?	?	-	-	-	-

Where

• GetInvocationList is the "current way" of getting the individual delegates
• Direct is just calling delegate.Invoke() (no individual control over invocation)
• Reflection.Emit is retrieving the _invocationList field of a delegate via Reflection.Emit (+- what is returned by GetInvocationList)
• Interface is a single method interface stored in a list
• List_Action is all induvidual delegates stored within a list and iterated over

And the first entry always being for one attached event handler and the 2nd one for two handlers (action += handler) (Or entries in the list)

I think the relevant notes are:

• GetInvocationList is kind of slow and allocates
• Direct is obviously unbeatable, but won't allow us to do what we want
• Emit is nice, a but not fast enough while also having additional dependency
• Interface / List_Action are reasonably fast while not allocating. (Downside though is the increased complexity to store them in the list + the allocation will take place there)

This issue should be taken as the place to dicuss where we want to move to with the events in our public API.

[bollhals]

I've done more investigation into this.
Since most of the event registration stays for the lifetime of the object and is invoked once or multiple times, I've investigated caching the array of GetInvocationList upon the first invoke.

Here's the outcome:

Invocation:

Method	Length	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated	Code Size
SingleInvoke	1	1.221 ns	0.2205 ns	0.0121 ns	1.00	0.00	-	-	-	-	26 B
Original_GetInvocationList	1	15.755 ns	1.2237 ns	0.0671 ns	12.91	0.18	0.0068	-	-	32 B	464 B
List	1	7.942 ns	0.6367 ns	0.0349 ns	6.51	0.04	-	-	-	-	305 B
Invoker	1	4.483 ns	0.2595 ns	0.0142 ns	3.67	0.04	-	-	-	-	259 B
SingleInvoke	2	7.847 ns	0.1600 ns	0.0088 ns	1.00	0.00	-	-	-	-	26 B
Original_GetInvocationList	2	33.112 ns	4.5988 ns	0.2521 ns	4.22	0.03	0.0085	-	-	40 B	464 B
List	2	12.264 ns	0.7842 ns	0.0430 ns	1.56	0.01	-	-	-	-	305 B
Invoker	2	7.124 ns	0.1031 ns	0.0056 ns	0.91	0.00	-	-	-	-	259 B

Add/remove Handler:

Method	count	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
Event	1	28.36 ns	1.782 ns	0.098 ns	1.00	-	-	-	-
Invoker	1	27.81 ns	1.100 ns	0.060 ns	0.98	-	-	-	-
List	1	28.39 ns	4.531 ns	0.248 ns	1.00	0.0136	-	-	64 B
Event	5	536.72 ns	107.891 ns	5.914 ns	1.00	0.1659	-	-	784 B
Invoker	5	511.23 ns	31.671 ns	1.736 ns	0.95	0.1659	-	-	784 B
List	5	204.34 ns	10.492 ns	0.575 ns	0.38	0.0527	-	-	248 B

Sources:
Invoker

internal struct EventActionInvoker<T>
    {
        private event Action<T>? _event;
        private Delegate[]? _handlers;
        private Action<Exception>? _onExceptionAction;

        public bool IsEmpty => _event is null;

        public EventActionInvoker(Action<Exception> onExceptionAction)
        {
            _event = null;
            _handlers = null;
            _onExceptionAction = onExceptionAction;
        }

        public void AddHandler(Action<T>? handler)
        {
            _event += handler;
            _handlers = null;
        }

        public void RemoveHandler(Action<T>? handler)
        {
            _event -= handler;
            _handlers = null;
        }

        public void Invoke(T parameter)
        {
            var handlers = _handlers;
            if (handlers is null)
            {
                handlers = _event?.GetInvocationList();
                if (handlers is null)
                {
                    return;
                }

                _handlers = handlers;
            }
            foreach (Action<T> action in handlers)
            {
                try
                {
                    action(parameter);
                }
                catch (Exception e)
                {
                    _onExceptionAction?.Invoke(e);
                }
            }
        }

        public void Takeover(in EventActionInvoker<T> other)
        {
            _event = other._event;
            _handlers = other._handlers;
            _onExceptionAction = other._onExceptionAction;
        }
    }

Bench - Add/Remove Handler

using System;
using System.Collections.Generic;
using BenchmarkDotNet.Attributes;
using RabbitMQ.Client.client.impl.Channel;

namespace Benchmarks.Eventing
{
    [ShortRunJob]
    [MemoryDiagnoser]
    public class Eventing_AddRemove
    {
        private readonly Action<ulong> _action = _ => { };
        private List<Action<ulong>> _list;
        private event Action<ulong> _event;
        private EventActionInvoker<ulong> _invoker;

        [Benchmark(Baseline = true)]
        [Arguments(1)]
        [Arguments(5)]
        public void Event(int count)
        {
            for (int i = 0; i < count; i++)
            {
                _event += _action;
            }

            for (int i = 0; i < count; i++)
            {
                _event -= _action;
            }
        }

        [Benchmark]
        [Arguments(1)]
        [Arguments(5)]
        public void Invoker(int count)
        {
            for (int i = 0; i < count; i++)
            {
                _invoker.AddHandler(_action);
            }

            for (int i = 0; i < count; i++)
            {
                _invoker.RemoveHandler(_action);
            }
        }

        [Benchmark]
        [Arguments(1)]
        [Arguments(5)]
        public void List(int count)
        {
            _list = new List<Action<ulong>>(1);
            for (int i = 0; i < count; i++)
            {
                _list.Add(_action);
            }

            for (int i = 0; i < count; i++)
            {
                _list.Remove(_action);
            }
        }
    }
}

Bench - Invoke

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
using RabbitMQ.Client.client.impl.Channel;

namespace Benchmarks.WireFormatting
{
    [ShortRunJob]
    [MemoryDiagnoser]
    [DisassemblyDiagnoser]
    public class Eventing_Invoke
    {
        private List<Action<ulong>> _list;
        private event Action<ulong> _event;
        private EventActionInvoker<ulong> _invoker;

        public Eventing_Invoke()
        {
            _invoker = new EventActionInvoker<ulong>(ex => OnUnhandledExceptionOccurred(ex, "ContextString"));
        }

        [Params(1, 2)]
        public int Length
        {
            set
            {
                _list = new List<Action<ulong>>(value);
                var action = new Action<ulong>(tag => { });
                for (int i = 0; i < value; i++)
                {
                    _event += action;
                    _invoker.AddHandler(action);
                    _list.Add(action);
                }
            }
        }

        [Benchmark(Baseline = true)]
        public void SingleInvoke()
        {
            _event?.Invoke(5UL);
        }

        [Benchmark]
        public void Original_GetInvocationList()
        {
            var handler = _event;
            if (handler != null)
            {
                foreach (Action<ulong> action in handler.GetInvocationList())
                {
                    try
                    {
                        action(5UL);
                    }
                    catch (Exception e)
                    {
                        OnUnhandledExceptionOccurred(e, "ContextString");
                    }
                }
            }
        }

        [Benchmark]
        public void List()
        {
            var handler = _event;
            if (handler != null)
            {
                foreach (Action<ulong> action in _list)
                {
                    try
                    {
                        action(5UL);
                    }
                    catch (Exception e)
                    {
                        OnUnhandledExceptionOccurred(e, "ContextString");
                    }
                }
            }
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        private void OnUnhandledExceptionOccurred(Exception exception, string context)
        {
        }

        [Benchmark]
        public void Invoker()
        {
            _invoker.Invoke(5UL);
        }
    }
}

Sidenote: The List approach is not thread safe while the others are. (Implementing it would degregate the performance to some degree I suppose)