Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CollectionView] [Windows] memory leaks when ItemsSource has changed #13393

Closed
ivan-todorov-progress opened this issue Feb 16, 2023 · 10 comments · Fixed by #13530
Closed

[CollectionView] [Windows] memory leaks when ItemsSource has changed #13393

ivan-todorov-progress opened this issue Feb 16, 2023 · 10 comments · Fixed by #13530
Assignees
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView fixed-in-7.0.81 Look for this fix in 7.0.81! fixed-in-7.0.92 Look for this fix in 7.0.92! fixed-in-7.0.100 fixed-in-7.0.101 fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! partner Issue or Request from a partner team platform/windows 🪟 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)

Comments

@ivan-todorov-progress
Copy link

ivan-todorov-progress commented Feb 16, 2023

Description

It seems that the CollectionView is leaking memory every time its ItemsSource property has changed.

Due to a lack of a proper memory profiler on all platforms, I am using a poor man's memory tracker implementation that stores a collection of WeakReferences and observes their IsAlive status:

public static class MemoryTracker
{
    private static readonly List<WeakReference> weakReferences = new List<WeakReference>();

    public static readonly BindableProperty IsTrackedProperty =
        BindableProperty.CreateAttached("IsTracked", typeof(bool), typeof(MemoryTracker),
            false, propertyChanged: OnIsTrackedChanged);

    public static int TotalObjectCount => weakReferences.Count;

    public static int AliveObjectCount => weakReferences.Count(weakReference => weakReference.IsAlive);

    public static bool GetIsTracked(BindableObject bindable)
    {
        return (bool)bindable.GetValue(IsTrackedProperty);
    }

    public static void SetIsTracked(BindableObject bindable, bool isTracked)
    {
        bindable.SetValue(IsTrackedProperty, isTracked);
    }

    private static void OnIsTrackedChanged(BindableObject bindable, object oldValue, object newValue)
    {
        if ((bool)newValue)
        {
            var weakReference = new WeakReference(bindable);

            weakReferences.Add(weakReference);
        }
    }
}

I am using this class to track visual components in XAML like this:

<Grid local:MemoryTracker.IsTracked="True">
    <Label Text="{Binding}" />
</Grid>

I have prepared a small sample project that demonstrates the memory leak.

Note: It is possible that the following bug is related: Memory leaks EVERYWHERE. Since I am describing a somewhat different scenario, I have decided to log a separate bug report for it.

Steps to Reproduce

  1. Build and run the sample project in Release build configuration.
  2. Click on the "Refresh Memory Info" button to show the initial memory statistics.
  3. Click on the "Refresh Items Source" button several times to recreate the collection of items.
  4. Click on the "Refresh Memory Info" button to update the memory statistics.
  5. Repeat the steps 3 and 4 several times to observe how both total tracked objects and alive objects constantly grow.
  6. Click the Recreate Control button to recreate the CollectionView in the visual tree.
  7. Click on the "Refresh Memory Info" button to update the memory statistics.
  8. Observe how the alive objects are decreased after the CollectionView is recreated.

Note: Clicking the "Refresh Memory Info" button calls the garbage collector forcefully before updating the memory statistics:

GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();

Link to public reproduction project repository

https://github.com/ivan-todorov-progress/maui-collection-view-memory-leak-bug

Version with bug

7.0 (current)

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

N/A

Did you find any workaround?

No response

Relevant log output

No response

@ivan-todorov-progress ivan-todorov-progress added the t/bug Something isn't working label Feb 16, 2023
@jsuarezruiz jsuarezruiz added partner Issue or Request from a partner team area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Feb 16, 2023
@jsuarezruiz jsuarezruiz added this to the Backlog milestone Feb 16, 2023
@ghost
Copy link

ghost commented Feb 16, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@jsuarezruiz
Copy link
Contributor

Could be related with #13327

@jonathanpeppers
Copy link
Member

Yes, I think some of these recent fixes might solve this one:

But I will test this case with main next week.

@jonathanpeppers jonathanpeppers self-assigned this Feb 17, 2023
@LanceMcCarthy
Copy link

Thanks for the fast response, Redth and Javier! When you're done reviewing, can you please come back here and let me know which of those items was the culprit? Much appreciated!

ZoolanderBossGIF

@samhouts samhouts added the legacy-area-perf Startup / Runtime performance label Feb 17, 2023
@minze-it
Copy link

Those leaks are a major issue and an absolute show stopper for our app causing the dotnet-maui version not going live. It would be so great to have those fixes in the next service release of the dotnet 7 version.

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Feb 23, 2023
Fixes: dotnet#13393

Context: https://github.com/ivan-todorov-progress/maui-collection-view-memory-leak-bug

Debugging the above sample, I found that `ItemsView._logicalChildren`
grew indefinitely in size if you replace a `CollectionView`'s
`ItemsSource` over and over.

I could fix this by creating a new `internal`
`ItemsView.ClearLogicalChildren()` method and call it from the Windows
`ItemsViewHandler`.

WIP to determine if we can write a test or if this happens on any
other platforms. Android seems OK.
@jonathanpeppers
Copy link
Member

@ivan-todorov-progress I did indeed find an issue with your sample app above: #13530

Do you know if this only happens on Windows? Android actually seems like it is OK without me changing anything:

image

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Feb 24, 2023
Fixes: dotnet#13393
Context: https://github.com/ivan-todorov-progress/maui-collection-view-memory-leak-bug

Debugging the above sample, I found that `ItemsView._logicalChildren`
grew indefinitely in size if you replace a `CollectionView`'s
`ItemsSource` over and over.

I could fix this by creating a new `internal`
`ItemsView.ClearLogicalChildren()` method and call it from the Windows
`ItemsViewHandler`.

I could reproduce this issue in a Device Test running on Windows.

It appears the problem does not occur on Android or iOS due to the
recycling behavior of the underlying platforms.

For example, Android calls `OnViewRecycled()` which calls
`RemoveLogicalChild(view)`:

* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/Adapters/ItemsViewAdapter.cs#L52-L57
* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/TemplatedItemViewHolder.cs#L37-L45

On Windows, we merely have a `ItemContentControl` and there is no
callback for when things are recycled. We *do* get a callback for when
the `FormsDataContext` value changes, so an option is to clear the
list in this case.

After this change, my test passes -- but it was already passing on iOS
and Android.
@ivan-todorov-progress
Copy link
Author

@jonathanpeppers I can confirm this is not happening on Android. My speculation is that on Android the CollectionView clears the BindingContext of its items when they are recycled, so the Bindings get disconnected and we have no memory leak. On Windows it is easily reproducible, however. I have not had the chance to test it with the latest version of Visual Studio and .NET MAUI to see whether the leak is fixed. I'll keep you posted with my findings, once I upgrade.

@ivan-todorov-progress ivan-todorov-progress changed the title CollectionView leaks memory when ItemsSource has changed [Windows] CollectionView leaks memory when ItemsSource has changed Feb 27, 2023
@ivan-todorov-progress ivan-todorov-progress changed the title [Windows] CollectionView leaks memory when ItemsSource has changed [CollectionView] [Windows] memory leaks when ItemsSource has changed Feb 27, 2023
@ivan-todorov-progress
Copy link
Author

ivan-todorov-progress commented Feb 27, 2023

@jonathanpeppers I have updated Visual Studio to the latest 17.5 version and re-tested the sample app on all platforms. On Android and iOS there is no memory leak. The memory leak on Windows is still there, though. I believe, we need your fix about ClearLogicalChildren as well: jonathanpeppers@c7837db.

I have updated the bug title and description to make it clear it is a Windows-only issue, in case someone else is tracking this.

P.S.: Great job fixing all these memory-related problems in .NET MAUI!

jonathanpeppers added a commit that referenced this issue Feb 27, 2023
…13530)

Fixes: #13393
Context: https://github.com/ivan-todorov-progress/maui-collection-view-memory-leak-bug

Debugging the above sample, I found that `ItemsView._logicalChildren`
grew indefinitely in size if you replace a `CollectionView`'s
`ItemsSource` over and over.

I could fix this by creating a new `internal`
`ItemsView.ClearLogicalChildren()` method and call it from the Windows
`ItemsViewHandler`.

I could reproduce this issue in a Device Test running on Windows.

It appears the problem does not occur on Android or iOS due to the
recycling behavior of the underlying platforms.

For example, Android calls `OnViewRecycled()` which calls
`RemoveLogicalChild(view)`:

* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/Adapters/ItemsViewAdapter.cs#L52-L57
* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/TemplatedItemViewHolder.cs#L37-L45

On Windows, we merely have a `ItemContentControl` and there is no
callback for when things are recycled. We *do* get a callback for when
the `FormsDataContext` value changes, so an option is to clear the
list in this case.

After this change, my test passes -- but it was already passing on iOS
and Android.
github-actions bot pushed a commit that referenced this issue Mar 2, 2023
Fixes: #13393
Context: https://github.com/ivan-todorov-progress/maui-collection-view-memory-leak-bug

Debugging the above sample, I found that `ItemsView._logicalChildren`
grew indefinitely in size if you replace a `CollectionView`'s
`ItemsSource` over and over.

I could fix this by creating a new `internal`
`ItemsView.ClearLogicalChildren()` method and call it from the Windows
`ItemsViewHandler`.

I could reproduce this issue in a Device Test running on Windows.

It appears the problem does not occur on Android or iOS due to the
recycling behavior of the underlying platforms.

For example, Android calls `OnViewRecycled()` which calls
`RemoveLogicalChild(view)`:

* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/Adapters/ItemsViewAdapter.cs#L52-L57
* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/TemplatedItemViewHolder.cs#L37-L45

On Windows, we merely have a `ItemContentControl` and there is no
callback for when things are recycled. We *do* get a callback for when
the `FormsDataContext` value changes, so an option is to clear the
list in this case.

After this change, my test passes -- but it was already passing on iOS
and Android.
github-actions bot pushed a commit that referenced this issue Mar 20, 2023
Fixes: #13393
Context: https://github.com/ivan-todorov-progress/maui-collection-view-memory-leak-bug

Debugging the above sample, I found that `ItemsView._logicalChildren`
grew indefinitely in size if you replace a `CollectionView`'s
`ItemsSource` over and over.

I could fix this by creating a new `internal`
`ItemsView.ClearLogicalChildren()` method and call it from the Windows
`ItemsViewHandler`.

I could reproduce this issue in a Device Test running on Windows.

It appears the problem does not occur on Android or iOS due to the
recycling behavior of the underlying platforms.

For example, Android calls `OnViewRecycled()` which calls
`RemoveLogicalChild(view)`:

* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/Adapters/ItemsViewAdapter.cs#L52-L57
* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/TemplatedItemViewHolder.cs#L37-L45

On Windows, we merely have a `ItemContentControl` and there is no
callback for when things are recycled. We *do* get a callback for when
the `FormsDataContext` value changes, so an option is to clear the
list in this case.

After this change, my test passes -- but it was already passing on iOS
and Android.
rmarinho pushed a commit that referenced this issue Mar 20, 2023
…View.ItemsSource` changes (#14075)

* [windows] fix memory leak when `CollectionView.ItemsSource` changes

Fixes: #13393
Context: https://github.com/ivan-todorov-progress/maui-collection-view-memory-leak-bug

Debugging the above sample, I found that `ItemsView._logicalChildren`
grew indefinitely in size if you replace a `CollectionView`'s
`ItemsSource` over and over.

I could fix this by creating a new `internal`
`ItemsView.ClearLogicalChildren()` method and call it from the Windows
`ItemsViewHandler`.

I could reproduce this issue in a Device Test running on Windows.

It appears the problem does not occur on Android or iOS due to the
recycling behavior of the underlying platforms.

For example, Android calls `OnViewRecycled()` which calls
`RemoveLogicalChild(view)`:

* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/Adapters/ItemsViewAdapter.cs#L52-L57
* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/TemplatedItemViewHolder.cs#L37-L45

On Windows, we merely have a `ItemContentControl` and there is no
callback for when things are recycled. We *do* get a callback for when
the `FormsDataContext` value changes, so an option is to clear the
list in this case.

After this change, my test passes -- but it was already passing on iOS
and Android.

* Fix for test on iOS

Apparently iOS is only creating 1:

    Assert.Equal() Failure\nExpected: 3\nActual:   1

* Auto-format source code

---------

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com>
@JanZeman
Copy link

Hello @jonathanpeppers, I know this issue is closed some weeks ago. Nevertheless, can you please shortly comment on my first question on this PR? Does you change fixes ObservableCollection behavior as well?

@JanZeman
Copy link

Hi @rmarinho, I can see this fix got merged to release/7.0.2xx. Can you please shortly explain or give reference to the release model? Is there any description that will explain which Visual Studio version will bring this fix to my local environment so I can try to verify and experiment? Thank you very much in advance.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 27, 2023
@samhouts samhouts modified the milestones: Backlog, .NET 8 May 24, 2023
@samhouts samhouts added fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! fixed-in-7.0.81 Look for this fix in 7.0.81! labels Jun 8, 2023
@samhouts samhouts modified the milestones: .NET 8, .NET 7 + Servicing Jul 10, 2023
@samhouts samhouts added the fixed-in-7.0.92 Look for this fix in 7.0.92! label Jul 11, 2023
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView fixed-in-7.0.81 Look for this fix in 7.0.81! fixed-in-7.0.92 Look for this fix in 7.0.92! fixed-in-7.0.100 fixed-in-7.0.101 fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! partner Issue or Request from a partner team platform/windows 🪟 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants