Closed GoldyD closed 5 years ago
Hi @GoldyD,
As you noted, the CollectDeleted
method is called in Raise
and Unsubscribe
. CollectDeleted
just "compacts" the delegate list to remove nulls ; so there's no need to call it in Subscribe
, because Subscribe
just adds a handler; it never introduces a new null
in the list.
Anyway, CollectDeleted
is just an optimization to ensure we don't have too many nulls in the list, which would make things inefficient. Not calling it does not prevent "dead" handlers from being collected.
Is there a concrete reason (other than your analysis of the code) why you think there's a memory leak? Do you see memory usage rising, with references tracking back to the WeakEventSource
?
In my code Unsubscribe and Raise method (that calls CollectDeleted method) is never called.
Sorry, I misread your comment. What's the point of subscribing to an event if you never raise it?
Anyway, just calling CollectDeleted
in Subscribe
won't be enough to solve your problem. CollectDeleted
compacts the list to remove invalidated handlers, but it doesn't invalidate them itself. Invalidate
only happens when the event is raised.
I can see how this would be a problem. If many short-lived handlers are added to an event that is rarely raised, the delegate list will keep growing and will end up containing mostly dead handlers.
On the other hand, I can't really scan the list for dead handlers every time Subscribe
is called; that would be very inefficient. Let me think about it; there must be a way to improve this.
In my code Unsubscribe and Raise method (that calls CollectDeleted method) is never called.
Sorry, I misread your comment. What's the point of subscribing to an event if you never raise it?
Event raised but too rarely - as example only when config file has changes
On the other hand, I can't really scan the list for dead handlers every time Subscribe is called; that would be very inefficient. Let me think about it; there must be a way to improve this.
We can call Scan on each one of 50 calls Subscribe
method as example
We can call Scan on each one of 50 calls
Subscribe
method as example
Yes, I was thinking of something like this. Or maybe something a little more subtle: scan every N calls to Subcribe
, unless Raise
has been called in between.
Hi @GoldyD, I just published a 4.0.1-beta.1 with a fix for this. Could you please try it to confirm it solves your problem?
Thanks!
Hi @thomaslevesque!
Great thanks for fix!
Issue with Dictionary<int, List
@GoldyD are you sure these handlers are not still referenced somewhere else?
Those handlers referenced from list _delegates
Those handlers referenced from list
_delegates
I mean your handlers (the ones passed to Subscribe
), not the internal handlers.
Are you sure you checked after a GC? If a GC didn't happen, the subscribers are not collected, so they're not removed from the list.
I just profiled this code, using dotMemory. Between two snapshots, I always have a "Objects delta" metrics for WeakDelegate
between about -50 and +50, so I don't think there's a leak.
Yes I check after GC done (call Force GC in dotMemory and Collect Allocation buttons).
Objects that has subscription on WeakEvent is collected, but WeakEventSource
object has memory leaks
@GoldyD this screenshot doesn't prove a memory leak. What do you see when you compare two snapshots?
Sorry, I run app with old nugget packet version :( I do test again
So what do you conclude from this?
There are 137 new WeakDelegate, but in itself, it doesn't mean anything. The handlers could just still be alive, in which case they're not removed. Or maybe CollectDeleted
(which I renamed to CompactHandlerList
) hasn't yet been called since the last GC. Hard to tell without seeing the code...
I simplified my test code (removed the GC.Collect calls and the sleeps), and I get this (diff between first and last snapshot)
Can't see any leak...
With _items
that hold WeakDelegate
all good - I add some items and list _items
collected.
But "_entries" seems not be collected
Please look at that code
public void CompactHandlerList()
{
...
if (_index[hashCode].Count == 0)
{
_index.Remove(hashCode);
}
I mean this never called, but not sure
I mean this never called, but not sure
What makes you say that? As you can see in my screenshot, 2809 List<int>
(the values in _index
) have been removed.
Yes _index
values removed but only on second call CompactHandlerList
method.
May be rewrite code block
foreach (var hashCode in hashCodes)
{
if (_index[hashCode].Count == 0)
{
_index.Remove(hashCode);
}
else
{
var oldIndexList = _index[hashCode];
var newIndexList = new List<int>(oldIndexList.Count);
foreach (var oi in oldIndexList)
{
if (newIndices.TryGetValue(oi, out int ni))
newIndexList.Add(ni);
}
_index[hashCode] = newIndexList;
}
}
To
foreach (var hashCode in hashCodes)
{
var oldIndexList = _index[hashCode];
var newIndexList = new List<int>(oldIndexList.Count);
foreach (var oi in oldIndexList)
{
if (newIndices.TryGetValue(oi, out int ni))
newIndexList.Add(ni);
}
if (newIndexList.Count > 0)
{
_index[hashCode] = newIndexList;
}
else
{
_index.Remove(hashCode);
}
}
Ah, I think I see what you mean. Sometimes I'm replacing the oldIndexList with a newIndexList that's empty. It's a good point. I'll fix that (not exactly how you suggested, but to the same end).
Thanks!
@GoldyD I published another beta
Thanks!
Let me know if it works for you, and I'll publish a stable version
Sorry for late answer. All works fine!
Cool, thanks! I'll release 4.0.1 then
Hello!
Please check DelegateCollectionBase class for memory leak issue in Dictionary
private readonly Dictionary<int, List<int>> _index;
I think this behavior because this method:
Newer called. In my code Unsubscribe and Raise method (that calls CollectDeleted method) is never called. For fix this issue I think need also add call CollectDeleted method in Subscribe method on class WeakEventSourceHelper too.