Skip to content

Commit

Permalink
[controls] fix memory leak in Style and AttachedCollection (#13260)
Browse files Browse the repository at this point in the history
Context: https://github.com/Vroomer/MAUI-master-detail-memory-leak
Context: https://github.com/symbiogenesis/Maui.DataGrid/tree/memory-leak
Context: #12039

Using the Visual Studio's `.NET Object Allocation Tracking` profiler
on the sample above I noticed after using the app and taking snapshots:

Snapshot 2:

    WeakReference<Microsoft.Maui.Controls.BindableObject>
    Count: 1,545
    Bytes: 37,080

Snapshot 10:

    WeakReference<Microsoft.Maui.Controls.BindableObject>
    Count: 1,657
    Bytes: 39,768

It appears memory usage is slowly growing by about ~2.45KB each time I
navigate forward a page then return.

f8171b4 introduced a `CleanupTrigger`, but the code has a slight issue:

    void CleanUpWeakReferences()
    {
        if (_targets.Count < _cleanupThreshold)
        {
            return;
        }

        _targets.RemoveAll(t => !t.TryGetTarget(out _));
        _cleanupThreshold = _targets.Count + CleanupTrigger;
    }

Because `_cleanupThreshold` is modified at the bottom of this method,
the `_targets` collection increases by 128 in size over time. The
result being lots of tiny `WeakReference<T>` objects in memory, but
not the actual underlying `T` target.

I did a little research to try to find a better collection than
`List<WeakReference<T>>`:

* https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2
* https://source.dot.net/#Microsoft.Extensions.Caching.Memory/MemoryCache.cs
* https://source.dot.net/#PresentationFramework/System/Windows/Input/KeyboardNavigation.cs,08271c29723aa10a

I did not find one that exactly met our needs, so I wrote a very
simple `WeakList<T>`. It cleans up `WeakReference<T>` that are no
longer alive after N operations. I set this threshold to 32 to start,
we can modify this later if needed. This makes the above samples call
it a couple times for large `CollectionView`'s, which seems reasonable.

After these changes, I'm able to navigate in the app over and over
with stable memory usage:

| ID | Objects | Heap Size     |
| -- | --:     | --:           |
| 1  | 107,885 | 407,645.87 KB |
| 2  |   2,267 | 401,126.22 KB |

This was after navigating through the app *many* times, and it
actually went *down*.

Then I found `Microsoft.Maui.Internals.WeakList<T>`, which I did not
like quite as much as my implementation. I deleted/replaced its usage
adding new unit tests for `WeakList<T>`.

Note that I believe I found one other leak, so this doesn't fully
solve #12039 yet.
  • Loading branch information
jonathanpeppers authored Feb 14, 2023
1 parent 918d1ce commit 23b9ae2
Show file tree
Hide file tree
Showing 7 changed files with 153 additions and 183 deletions.
55 changes: 8 additions & 47 deletions src/Controls/src/Core/Interactivity/AttachedCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,7 @@ namespace Microsoft.Maui.Controls
{
internal class AttachedCollection<T> : ObservableCollection<T>, ICollection<T>, IAttachedObject where T : BindableObject, IAttachedObject
{
readonly List<WeakReference> _associatedObjects = new List<WeakReference>();

const int CleanupTrigger = 128;
int _cleanupThreshold = CleanupTrigger;
readonly WeakList<BindableObject> _associatedObjects = new();

public AttachedCollection()
{
Expand Down Expand Up @@ -38,13 +35,10 @@ public void DetachFrom(BindableObject bindable)

protected override void ClearItems()
{
foreach (WeakReference weakbindable in _associatedObjects)
foreach (var bindable in _associatedObjects)
{
foreach (T item in this)
{
var bindable = weakbindable.Target as BindableObject;
if (bindable == null)
continue;
item.DetachFrom(bindable);
}
}
Expand All @@ -54,11 +48,8 @@ protected override void ClearItems()
protected override void InsertItem(int index, T item)
{
base.InsertItem(index, item);
foreach (WeakReference weakbindable in _associatedObjects)
foreach (var bindable in _associatedObjects)
{
var bindable = weakbindable.Target as BindableObject;
if (bindable == null)
continue;
item.AttachTo(bindable);
}
}
Expand All @@ -67,8 +58,7 @@ protected virtual void OnAttachedTo(BindableObject bindable)
{
lock (_associatedObjects)
{
_associatedObjects.Add(new WeakReference(bindable));
CleanUpWeakReferences();
_associatedObjects.Add(bindable);
}
foreach (T item in this)
item.AttachTo(bindable);
Expand All @@ -80,27 +70,15 @@ protected virtual void OnDetachingFrom(BindableObject bindable)
item.DetachFrom(bindable);
lock (_associatedObjects)
{
for (var i = 0; i < _associatedObjects.Count; i++)
{
object target = _associatedObjects[i].Target;

if (target == null || target == bindable)
{
_associatedObjects.RemoveAt(i);
i--;
}
}
_associatedObjects.Remove(bindable);
}
}

protected override void RemoveItem(int index)
{
T item = this[index];
foreach (WeakReference weakbindable in _associatedObjects)
foreach (var bindable in _associatedObjects)
{
var bindable = weakbindable.Target as BindableObject;
if (bindable == null)
continue;
item.DetachFrom(bindable);
}

Expand All @@ -110,34 +88,17 @@ protected override void RemoveItem(int index)
protected override void SetItem(int index, T item)
{
T old = this[index];
foreach (WeakReference weakbindable in _associatedObjects)
foreach (var bindable in _associatedObjects)
{
var bindable = weakbindable.Target as BindableObject;
if (bindable == null)
continue;
old.DetachFrom(bindable);
}

base.SetItem(index, item);

foreach (WeakReference weakbindable in _associatedObjects)
foreach (var bindable in _associatedObjects)
{
var bindable = weakbindable.Target as BindableObject;
if (bindable == null)
continue;
item.AttachTo(bindable);
}
}

void CleanUpWeakReferences()
{
if (_associatedObjects.Count < _cleanupThreshold)
{
return;
}

_associatedObjects.RemoveAll(t => t == null || !t.IsAlive);
_cleanupThreshold = _associatedObjects.Count + CleanupTrigger;
}
}
}
32 changes: 5 additions & 27 deletions src/Controls/src/Core/Style.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,10 @@ public sealed class Style : IStyle
{
internal const string StyleClassPrefix = "Microsoft.Maui.Controls.StyleClass.";

const int CleanupTrigger = 128;
int _cleanupThreshold = CleanupTrigger;

readonly BindableProperty _basedOnResourceProperty = BindableProperty.CreateAttached("BasedOnResource", typeof(Style), typeof(Style), default(Style),
propertyChanged: OnBasedOnResourceChanged);

readonly List<WeakReference<BindableObject>> _targets = new List<WeakReference<BindableObject>>(4);
readonly WeakList<BindableObject> _targets = new();

Style _basedOnStyle;

Expand Down Expand Up @@ -66,10 +63,8 @@ public string BaseResourceKey
return;
_baseResourceKey = value;
//update all DynamicResources
foreach (WeakReference<BindableObject> bindableWr in _targets)
foreach (var target in _targets)
{
if (!bindableWr.TryGetTarget(out BindableObject target))
continue;
target.RemoveDynamicResource(_basedOnResourceProperty);
if (value != null)
target.SetDynamicResource(_basedOnResourceProperty, value);
Expand Down Expand Up @@ -98,14 +93,12 @@ void IStyle.Apply(BindableObject bindable)
{
lock (_targets)
{
_targets.Add(new WeakReference<BindableObject>(bindable));
_targets.Add(bindable);
}

if (BaseResourceKey != null)
bindable.SetDynamicResource(_basedOnResourceProperty, BaseResourceKey);
ApplyCore(bindable, BasedOn ?? GetBasedOnResource(bindable));

CleanUpWeakReferences();
}

/// <include file="../../docs/Microsoft.Maui.Controls/Style.xml" path="//Member[@MemberName='TargetType']/Docs/*" />
Expand All @@ -117,7 +110,7 @@ void IStyle.UnApply(BindableObject bindable)
bindable.RemoveDynamicResource(_basedOnResourceProperty);
lock (_targets)
{
_targets.RemoveAll(wr => wr != null && wr.TryGetTarget(out BindableObject target) && target == bindable);
_targets.Remove(bindable);
}
}

Expand Down Expand Up @@ -149,11 +142,8 @@ void ApplyCore(BindableObject bindable, Style basedOn)

void BasedOnChanged(Style oldValue, Style newValue)
{
foreach (WeakReference<BindableObject> bindableRef in _targets)
foreach (var bindable in _targets)
{
if (!bindableRef.TryGetTarget(out BindableObject bindable))
continue;

UnApplyCore(bindable, oldValue);
ApplyCore(bindable, newValue);
}
Expand Down Expand Up @@ -183,17 +173,5 @@ void UnApplyCore(BindableObject bindable, Style basedOn)

bool ValidateBasedOn(Style value)
=> value is null || value.TargetType.IsAssignableFrom(TargetType);

void CleanUpWeakReferences()
{
if (_targets.Count < _cleanupThreshold)
return;

lock (_targets)
{
_targets.RemoveAll(t => t == null || !t.TryGetTarget(out _));
_cleanupThreshold = _targets.Count + CleanupTrigger;
}
}
}
}
3 changes: 1 addition & 2 deletions src/Core/src/HotReload/HotReloadHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.Reflection;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Maui.Hosting;
using Microsoft.Maui.Internal;

namespace Microsoft.Maui.HotReload
{
Expand Down Expand Up @@ -155,7 +154,7 @@ static void RegisterHandler(KeyValuePair<Type, Type> pair, Type newHandler)

public static void TriggerReload()
{
List<IHotReloadableView?>? roots = null;
List<IHotReloadableView>? roots = null;
while (roots == null)
{
try
Expand Down
106 changes: 0 additions & 106 deletions src/Core/src/HotReload/WeakList.cs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
using System;
using Microsoft.Maui.Devices;
using Microsoft.Maui.Internal;

namespace Microsoft.Maui.Platform
{
Expand Down
Loading

0 comments on commit 23b9ae2

Please sign in to comment.