Performance Issue - unsubscribing events

Question

In my application I noticed that my way of handling Events is causing performance issues.

I would like to know if thats to be expected, maybe I am doing something wrong there. Is there a way to fix my problem?

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {
            var x = new Main();
            x.Init();

            Console.ReadLine();
        }
    }

    public class Main
    {
        private Bar _bar;
        private List<Foo> _foos;

        public Main()
        {
            _bar = new Bar();
        }

        public void Init()
        {
            var sw = new Stopwatch();

            sw.Restart();
            _foos = new List<Foo>();
            for (int i = 0; i < 10000; i++)
            {
                var newFoo = new Foo();
                newFoo.Bar = _bar;
                _foos.Add(newFoo);
            }
            sw.Stop();

            Console.WriteLine("Init 10.000 Foos WITH un-subscribe event: {0} ms", sw.ElapsedMilliseconds);
            _foos.Clear();

            sw.Restart();
            _foos = new List<Foo>();
            for (int i = 0; i < 10000; i++)
            {
                var newFoo = new Foo();
                newFoo.BarWithout = _bar;
                _foos.Add(newFoo);
            }
            sw.Stop();

            Console.WriteLine("Init 10.000 Foos WITHOUT un-subscribe event: {0} ms", sw.ElapsedMilliseconds);
            _foos.Clear();
        }
    }

    public class Bar
    {
        public event EventHandler<string> Stuff;

        protected virtual void OnStuff(string e)
        {
            var stuff = this.Stuff;
            if (stuff != null)
                stuff(this, e);
        }
    }

    public class Foo
    {
        private Bar _bar;

        public Bar Bar
        {
            get { return _bar; }
            set
            {
                if (_bar != null)
                {
                    _bar.Stuff -= _bar_Stuff;
                }

                _bar = value;

                if (_bar != null)
                {
                    _bar.Stuff -= _bar_Stuff;
                    _bar.Stuff += _bar_Stuff;
                }
            }
        }

        public Bar BarWithout
        {
            get { return _bar; }
            set
            {
                if (_bar != null)
                {
                    //_bar.Stuff -= _bar_Stuff;    
                }

                _bar = value;

                if (_bar != null)
                {
                    //_bar.Stuff -= _bar_Stuff;
                    _bar.Stuff += _bar_Stuff;
                }
            }
        }

        private void _bar_Stuff(object sender, string e)
        {

        }
    }
}

In this sample code, my Foo class has 2 properties Bar and BarWithout. The BarWithout property has the un-subscribing out commented.

In the Main class the Init method I am creating 2 times 10.000 Foo objects and the first routine sets the Bar property the second sets the BarWithout property. On my machine the first routine takes ~2200 milliseonds and the second routine takes ~5ms.

Since the gap is kinda huge, I am wondering if there is a more efficient way of removing an Event handler?

Btw, yes I know I could change the code so that Main subscribes to the Event of Bar and than calls a method for all Foo objects in the list, kinda hope there is something "easier" without the need to refactor the current situation.

Edit:

With 4 times the data (so 40.000 instead of 10.000) the first routine already takes ~28.000 milliseconds compared to ~20 milliseconds, so the first routine is more than 10 times slower with only 4 times more data. The second routine stays constant with the Performance 4 times more data = 4 times slower.

Answer 1

You can back an event with a HashSet like this

private readonly HashSet<EventHandler> _eventHandlers = new HashSet<EventHandler>();
public event EventHandler MyEvent
{
    add => _eventHandlers.Add(value);
    remove => _eventHandlers.Remove(value);
}

protected virtual void OnMyEvent()
{
    foreach (EventHandler eventHandler in _eventHandlers.ToList())
    {
        eventHandler.Invoke(this, EventArgs.Empty);
    }
}

This will only give a significant performance boost with many event subscriptions. and it is not possible to have multiple subscriptions with the same event handler. You could implement that with Dictionary<EventHandler, List<EventHandler>>.

Answer 2

Since the gap is kinda huge, I am wondering if there is a more efficient way of removing an Event handler?

In general - no. In this particular case - yes. Just remove that code. It's redundant.

Just think a bit. The property setter should be the only place where you unsubscribe from the previous event source and subscribe to the new one. Thus, there is absolutely no need to unsubscribe from the new one (because you know your object should not subscribed) and what are you doing is no op. But of course the -= operation does not have that knowledge and have to go through the whole list of handlers just to find that there is nothing to remove. This is the worst case for every linear search algorithm and leads to O(N^2) time complexity when used in a loop, hence the difference in the performance.

The correct implementation should be something like this

public Bar Bar
{
    get { return _bar; }
    set
    {
        if (ReferenceEquals(_bar, value)) return; // Nothing to do
        if (_bar != null) _bar.Stuff -= _bar_Stuff;
        _bar = value;
        if (_bar != null) _bar.Stuff += _bar_Stuff;
     }
} 

Answer 3

Let’s take a look at what you’re actually doing in your loop:

var newFoo = new Foo();
newFoo.Bar = _bar;

So you create a new Foo everytime and assign an (existing) bar to it—which causes the Foo to attach an event handler.

In any case, there is never a Foo which already has a Bar assigned. So there never happens a deregistration of the event handler on the “old” Bar object, since there is no old Bar object. So the following condition at the beginning of your setter is never true and the code doesn’t run:

if (_bar != null)
{
    _bar.Stuff -= _bar_Stuff;
}

In every iteration _bar is null, so commenting out that line does not make any difference.

That leaves the only difference between Bar and BarWithout the following part:

if (_bar != null)
{
    _bar.Stuff -= _bar_Stuff;
    _bar.Stuff += _bar_Stuff;
}

This always runs since we always assign a non-null Bar to it. The event attaching also always runs so that can’t make the difference. Which leaves only the unregistration. And at that point I ask you: What do you expect that to do? Why do you unregister the same event handler you register directly afterwards?

Do you try this as an attempt to unregister event handlers from other Foos? That won’t work; the _bar_Stuff is specific to the current instance you’re in, so it cannot be another Foo’s handler.

So since _bar_Stuff is always the event handler of the Foo instance, and since there is always a new Foo that means that Bar will never have that event handler registered at that point. So the line attempts to remove an event handler that was never registered. And as your benchmark shows, that seems to be expensive, so you should avoid it.

Note that your benchmark also has another issue which is the _foos.Clear(). While this will clear the list and remove the references to the foos, the one Bar instance still has those event handlers registered. That means that the Bar keeps a reference to every Foo object, preventing them from being garbage collected. Further, the more often you run your loop, the more event handlers are registered, so unsubscribing an event handler that was not subscribed from the Bar will take even more time (you can easily see that if first run the BarWithOut benchmark).

So the tl;dr of all this is that you should make sure that the Foos unsubscribe from the event properly.