Feedback UnityEvent RemoveListener pointlessly allocates two Lists

The UnityEvent reference source code includes this function in `InvokableCallList`, which is executed every time we call `RemoveListener()` on a UnityEvent:

Code (CSharp):

1.  
2.         public void RemoveListener(object targetObj, MethodInfo method)
3.         {
4.             var toRemove = new List<BaseInvokableCall>();
5.             for (int index = 0; index < m_RuntimeCalls.Count; index++)
6.             {
7.                 if (m_RuntimeCalls[index].Find(targetObj, method))
8.                     toRemove.Add(m_RuntimeCalls[index]);
9.             }
10.             m_RuntimeCalls.RemoveAll(toRemove.Contains);
11.  
12.             // removals are done synchronously to avoid leaks
13.             var newExecutingCalls = new List<BaseInvokableCall>(m_PersistentCalls.Count + m_RuntimeCalls.Count);
14.             newExecutingCalls.AddRange(m_PersistentCalls);
15.             newExecutingCalls.AddRange(m_RuntimeCalls);
16.             m_ExecutingCalls = newExecutingCalls;
17.             m_NeedsUpdate = false;
18.         }

This seems to be unneccesarily allocating two new Lists (which then internally allocate two new arrays). Calling `RemoveAll()` with `Contains` as the predicate is also very inefficient.

The function could be rewritten like this to avoid any new allocations or inefficient use of RemoveAll:

Code (CSharp):

1.  
2.         public void RemoveListener(object targetObj, MethodInfo method)
3.         {
4.             //iterate backwards so removing an item from an index doesn't disrupt iteration
5.             for (int index = m_RuntimeCalls.Count - 1; index >= 0; index--)
6.             {
7.                 if (m_RuntimeCalls[index].Find(targetObj, method))
8.                     m_RuntimeCalls.RemoveAt(i);
9.             }
10.  
11.             // removals are done synchronously to avoid leaks. we reuse the existing list
12.             m_ExecutingCalls.Clear();
13.             m_ExecutingCalls.AddRange(m_PersistentCalls);
14.             m_ExecutingCalls.AddRange(m_RuntimeCalls);
15.             m_NeedsUpdate = false;
16.         }
17.  

There are other places in the file where `m_ExecutingCalls` is recreated as a new List instead of simply cleared, each with the same comment "removals are done synchronously to avoid leaks". Perhaps I'm misunderstanding the original programmer's meaning/intent, but `list.Clear()` is still synchronous, and if necessary the `Capacity` property or `TrimExcess` function can be used to change the capacity of the list.

 

Hi;

I am not highly experienced in the field but it appears using Clear, clears the references of all objects and sets the list count to 0. But it does not free the resources up, whom continue to maintain the old references until they overwrite.

using new list resets the memory used by the previous contents of that list to nothing. Freeing those resources. Otherwise at a bottleneck those resources may not be considered free.

but, hey, I will write this comment, but I am no pro here! So, if you know better than I about it, and can measure notable increase of performance or a reduction of GC then ignore this keep doing what you’re doing.

 

Sorry to rephrase that


If the contents of the previous list was very large, the next list maintains the memory cost of the previous list. If you cleared it. Rather than.. ‘newed’ it

 

As I said, you can avoid that simply by setting the Capacity property or calling the TrimExcess() function.

 

You’re right. Keep doing it: I believe in you.

You are reducing some percentage of garbage! Because any counts and capacities in the following list that remained count and capacity but were overwritten were not part of the disposal. (Using trim excess)

Very clever stuff!
I will certainly use!

 

Your proposed change would cause issues in a mulithreaded environment, especially when the removal happens from inside one of the callbacks. The comment specifically states they do this to change is synchronously. You kept the comment but it's not correct anymore. You change the list 3 times in a row (Clear and two AddRange which may also do several changes in a row. Replacing the entire list can be done in a single atomic step, so it would be thread safe.

If you worry about garbage, you should not change the Capacity manually in between, as any capacity change will allocate a new internal array. The point of using a list is to keep a larger internal array, so you don't need to re-allocate the array all the time.

Events shouldn't be added / removed too frequent. If something happens very often, it's not really an event

 

Well, that's why I said

The comment in the code says "synchronously"; it doesn't mention thread safety or concurrency. Methods like "Clear()" and "AddRange()" are synchronous (in the sense that they aren't asynchronous functions or coroutines), but you're correct that being synchronous doesn't mean that they're thread safe. The original code makes more sense in the context of thread safety. Thanks for clarifying.

 

Not only thread safety. Have a look at how the event is invoked. "PrepareInvoke" returns "m_ExecutingCalls" and it iterates through the list when invoking all subscribed events. As I said, when you remove an event from inside a subscribed event, the iteration would break as your changed code would directly manipulate the used list. The original code would create a new list and replace the old one. That means the iteration inside Invoke would continue with the old list that is stored in a local variable.

 

Yep, I see what you mean.