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

[windows] fix memory leak when CollectionView.ItemsSource changes #13530

Merged
merged 3 commits into from Feb 27, 2023

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Feb 23, 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):

public override void OnViewRecycled(Object holder)
{
if (holder is TemplatedItemViewHolder templatedItemViewHolder)
{
templatedItemViewHolder.Recycle(ItemsView);
}

public void Recycle(ItemsView itemsView)
{
if (View == null)
{
return;
}
itemsView.RemoveLogicalChild(View);
}

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.

@jonathanpeppers
Copy link
Member Author

It appears there is no issue on Android, iOS, or Catalyst:

image

image

Going to investigate why this is only needed on Windows.

@jonathanpeppers
Copy link
Member Author

Ok Android works, because there is a callback for "recycling":

public override void OnViewRecycled(Object holder)
{
if (holder is TemplatedItemViewHolder templatedItemViewHolder)
{
templatedItemViewHolder.Recycle(ItemsView);
}

public void Recycle(ItemsView itemsView)
{
if (View == null)
{
return;
}
itemsView.RemoveLogicalChild(View);
}

I'll continue looking tomorrow, it appears that Windows uses this ItemContentControl, and there is no callback when things are "recycled":

var itemContentControl = (ItemContentControl)d;
itemContentControl.Realize();

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.
jonathanpeppers and others added 2 commits February 24, 2023 13:21
Apparently iOS is only creating 1:

    Assert.Equal() Failure\nExpected: 3\nActual:   1
@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formating your source code to follow our code guidelines.

@jonathanpeppers jonathanpeppers merged commit f485316 into dotnet:main Feb 27, 2023
@jonathanpeppers jonathanpeppers deleted the WindowsItemSourceLeaks branch February 27, 2023 14:39
@JanZeman
Copy link

Thank you @jonathanpeppers for this fix. I am afraid I do not know the merging strategy but to my understanding the main branch will become available as 8-preview. Is there any chance these important memory-related fixes go backported into net-7? Or what is my chance to test it within my projects in the foreseeable future?

@jonathanpeppers jonathanpeppers added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Feb 28, 2023
@jonathanpeppers
Copy link
Member Author

I don't know how the backporting works for MAUI (I am on the Android team). This one seems like very minimal risk; I put the label so they can take a look.

@JanZeman
Copy link

Thank you!

@PureWeen
Copy link
Member

PureWeen commented Mar 2, 2023

/backport to net7.0

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/4315685016

@PureWeen PureWeen added the backport/approved After some discussion or review, this PR or change was approved to be backported. label Mar 2, 2023
PureWeen added a commit that referenced this pull request Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/collectionview 📃 CollectionView, CarouselView, IndicatorView backport/approved After some discussion or review, this PR or change was approved to be backported. backport/suggested The PR author or issue review has suggested that the change should be backported. memory-leak 💦 Memory usage grows / objects live forever platform/windows 🪟
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CollectionView] [Windows] memory leaks when ItemsSource has changed
6 participants