dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.04k stars 1.73k forks source link

Memory leak in RadioButton ControlTemplate #23199

Closed krdmllr closed 3 months ago

krdmllr commented 3 months ago

Description

Hi, we found that when refreshing a list which contains customized RadioButtons, hundreds of Ellipse objects from the template remain in memory. I created a sample repo which just uses the control template from the maui documentation https://learn.microsoft.com/en-us/dotnet/maui/user-interface/controls/radiobutton?view=net-maui-8.0#redefine-radiobutton-appearance

When refreshing the list a few times, doing GC.Collects in between and taking a snapshot just using Visual Studio, I found the same issue occuring in the sample repo: image

Same behavior happens with a Release build + JetBrains dotMemory in our production app.

Steps to Reproduce

  1. Start app
  2. Click "Recreate List" button, this does a GC.Collect(), clears and fills the list again

Link to public reproduction project repository

https://github.com/krdmllr/MauiMemoryLeaks

Version with bug

8.0.60 SR6

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

windows 10.0.19041.0

Did you find any workaround?

No response

Relevant log output

No response

github-actions[bot] commented 3 months ago

Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!

Open similar issues:

Closed similar issues:

Note: You can give me feedback by thumbs upping or thumbs downing this comment.

RoiChen001 commented 3 months ago

I can repro this issue at Windows platform on the latest 17.11.0 Preview 2.1(8.0.60).

jonathanpeppers commented 3 months ago

After deeper investigation, I don't think I'm seeing a problem here on Windows.

First, I would recommend these changes in the sample to test this:

The sample was calling ObservableCollection.Add() over and over, which is not the best for performance. It can be a bit better to replace the entire collection in one go.

With this change, I see memory being pretty stable and going up and down, even if I click "Recreate List" very quickly:

image

I don't see extra Ellipse instances alive here.

To be sure there wasn't a problem, I wrote a couple new tests in .NET MAUI, but they pass successfully:

Let me know if I missed something, or if I should try a different platform. Thanks!

krdmllr commented 3 months ago

Hi, thank you very much for taking a look!

I will retest that in our environment to see if anything changes.

We replace the whole collection and just use an array instead of ObservableCollection in our app. However, we also use a lot of rx with DynamicData where we filter and sort lists and bind them using ReadOnlyObservableCollections. They use Reset when swapping larger collections. Would you generally recommend to change that to replace the collections instead?

jonathanpeppers commented 3 months ago

The main thing to check is that an event fires once, instead of ObservableCollection.CollectionChanged firing 1-per item.

In my example, it was actually OnPropertyChanged(nameof(Items)) that refreshed the screen.

krdmllr commented 3 months ago

Can confirm that calling GC more often shows that the object count stays at 80 ellipses.

The overall memory consumption still increases when reloading the data, is that something that is expected?

image

jonathanpeppers commented 3 months ago

If you are always seeing 80, it's probably not a leak. It just means that it takes a few GCs for them to go away. A leak would mean 80->100->120 that continuously grows.

In my screenshot above, if I let the app "rest" for a second, that is what caused this drop to appear:

image

If you are testing and see something different than me, see if you can find out which object type is growing. I can take another look in that case, thanks!

krdmllr commented 3 months ago

If you are always seeing 80, it's probably not a leak. It just means that it takes a few GCs for them to go away. A leak would mean 80->100->120 that continuously grows.

Yes, I see that the ellipse is not leaking like I suggested initially, I have the same results after adding the additional GC calls. I was wondering why the heap size is still increasing between the snapshots (+200kb in my snapshot), should it not go back to 0 increase or even decrease when calling GC manually?

jonathanpeppers commented 3 months ago

For me, it was going down (lower than a previous collection) as I waited longer & clicked longer. There was a stable point the memory usage would hover around.

If you see it going up, diff a few snapshots to see if you can figure out what object (full namespace + type) is the cause.

krdmllr commented 3 months ago

@jonathanpeppers thank you, I will close the issue for now and raise a new one if necessary.