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

ListView/CollectionView memory leak under Windows #9162

Closed
hbraasch opened this issue Aug 3, 2022 · 20 comments · Fixed by #13333
Closed

ListView/CollectionView memory leak under Windows #9162

hbraasch opened this issue Aug 3, 2022 · 20 comments · Fixed by #13333
Assignees
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView area-controls-listview ListView and TableView 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.1.7762 Look for this fix in 8.0.0-preview.1.7762! p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint platform/windows 🪟 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)

Comments

@hbraasch
Copy link

hbraasch commented Aug 3, 2022

Description

A simple repro (see link below) demonstrates a ListView/CollectionView memory leak under Windows.
Each time the ItemSource gets updated with the exact same list, the app memory usage increases.
The repro is a simplification, normally the ListView/CollectionView gets filled through a binding. However the repro still demonstrates the same problem.

See this video for a demonstration:

ListviewMemoryTester.mp4

I'm a VS Community user so do not have access to the Xamarin Profiler to see if the problem also exists in Android and iOS.

Steps to Reproduce

  1. Open the app from the repro in VS: https://github.com/hbraasch/ListViewMemoryTester.git
  2. Set the platform to [Windows] and run
  3. Click on the refresh button in the toolbar, see how the memory usage increases
  4. Click on the [Click to launch [CollectionView] page] button, which launches the page based on a CollectionView
  5. Click on the refresh button in the toolbar, see how the memory usage also increases

Version with bug

6.0.408 (current)

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

net6.0-windows10.0.19041.0

Did you find any workaround?

No

Relevant log output

No response

@hbraasch hbraasch added s/needs-verification Indicates that this issue needs initial verification before further triage will happen t/bug Something isn't working labels Aug 3, 2022
@jfversluis
Copy link
Member

Thanks for the report and investigation on this!

I'm not necessarily the expert on performance testing, but what happens if you try calling the garbage collector? I can imagine that memory might still be growing depending on the memory that is available on the system and only flushed whenever necessary?

Besides this repro do you have any real-world scenarios where you had this happening? What made you start investigating this?

@jfversluis jfversluis added platform/windows 🪟 area-controls-collectionview CollectionView, CarouselView, IndicatorView investigate s/needs-info Issue needs more info from the author and removed s/needs-verification Indicates that this issue needs initial verification before further triage will happen labels Aug 3, 2022
@ghost
Copy link

ghost commented Aug 3, 2022

Hi @hbraasch. We have added the "s/needs-info" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@hbraasch
Copy link
Author

hbraasch commented Aug 3, 2022

Hi @jfversluis

When add the following code to the [Refresh] event, it now tries to force a GC before updating the ImageSource.

ToolbarItems.Add(new ToolbarItem("Refresh", "ic_refresh.png", () => {
    GC.Collect();
    listView.ItemsSource = GenerateDisplayItems();
}));

The small yellow dots on the memory graph shows where the GC's occurs. They do happen where expected. Unfortunately it still shows the memory growth

image

My real-world application... I'm developing a custom TreeView which inherits from a ListView. It appears to work well when running under Android and iOS (but I must admit I did not check for memory leaks as I do not have the tools - I think). But when I started testing for Windows, the Windows app crashes with an out-of-memory exception.

image

Since VS shows a memory graph while running a Windows app, I quickly noticed the continuous increase in memory each time e.g. a row gets expanded/collapsed, which is done by simply replacing the listview's ItemSource with a newly calculated (but mostly similar sized) one.

To investigating this further I started with a simple app (the repro), and found that it shows the same disconcerning effect.

I might be barking at the wrong tree, but I though I need to raise this before wasting an enormous amount of time searching for bugs elsewhere.

Regards

@ghost ghost added s/needs-attention Issue has more information and needs another look and removed s/needs-info Issue needs more info from the author labels Aug 3, 2022
@jfversluis jfversluis added legacy-area-perf Startup / Runtime performance and removed area-controls-collectionview CollectionView, CarouselView, IndicatorView s/needs-attention Issue has more information and needs another look labels Aug 4, 2022
@jfversluis jfversluis added this to the Backlog milestone Aug 4, 2022
@xtuzy
Copy link

xtuzy commented Sep 1, 2022

I also feel ListView have memory leak, it recycle item?
I make a example show 500 TextCell, i don't know why TextCell will have 1500 in Memory Snapshot after i scroll ListView.

sharex-20220901093408.mp4

@xtuzy
Copy link

xtuzy commented Sep 1, 2022

I add a static int to show count of new itemview, the count still grow.

sharex-20220901114011.mp4

@xtuzy
Copy link

xtuzy commented Sep 1, 2022

Change some code of ViewCell, ListView will recycle Cell at Android, but still have bug at Windows, still create new Cell, not only 500.

image

Example source code:
MauiAppListViewMemoryLeakBug1.zip

@RealGregor
Copy link

This bug should be a high priority to fix. The application is literally useless without the ability to scroll through content. And memory leak inside such a structure is a deterrent from even using .NET MAUI to develop anything.

@Keflon
Copy link

Keflon commented Sep 12, 2022

I too have written a TreeView and also tried composing it using a CollectionView and a ListView and ran into the same problem here: #8151 as well as a layout problem: #4520
As a workaround I wrote a virtualizing ListView I'm using until the CollectionView is fixed.
@hbraasch you may benefit from a similar approach, or you could save yourself some time and use my ListView, or the TreeView it was written for. Both are here: https://github.com/Keflon/FunctionZero.Maui.Controls
@jfversluis this and #8151 appear to be duplicates so you may want to close one?

@hbraasch
Copy link
Author

@Keflon Thanks

@ggutschi
Copy link

ggutschi commented Oct 21, 2022

This also happens in a FlexLayout with a BindableLayout.ItemsSource set (see sample attached).
MauiApp1.zip

And here is the object diff after two minutes:
image

@feal87
Copy link

feal87 commented Dec 17, 2022

Can confirm the issue is still here. Application memory usage just keeps going up endlessly as items are added/removed.

I worked around it for now by simply having a single ObservableCollection bound to the collectionView and Clear+AddRange from this collection whenever I need to reload the data.

@Odaronil
Copy link

Odaronil commented Dec 19, 2022

+1, It really annoys me when it takes up a lot of memory, especially if you add pictures there, the application already takes up more than 1 GB in less than 10 minutes!

PS: I also found a temporary solution, as the @feal87 on top wrote this - ObservableCollection - Clear + Add. But it only helps half.

@lucas-magalhaes-BTG
Copy link

Same issue here, any update or workaround?

@ghost
Copy link

ghost commented Jan 6, 2023

hello,
sorry im a noob programmer
only very little experience with C# but im making my first app
i too stumbled upon this problem
my app has a page that holds a collectionview that was connected to a List at first
there is functionality on the page to filter items by category, i did this by clearing the list and then adding in a loop (no idea if this is the proper way to do it but visually it works)
i would refresh the list by reassigning the ItemsSource property constantly
this led to memory leak, memory usage would just keep climbing endlessly everytime i did this. everytime the ItemsSource was refreshed, memory usage climbed by 10-20mb and would never decrease

earlier in the week started using ObservableCollection, which means not needing to reassign ItemsSource constantly, a little better on the memory usage it seems but still the leak is there. also OC has no AddRange function which i kinda need. adding items to OC can only be done 1 at a time, which apparently raises 3 events, this is unnecessary for my needs bc i continually clear and add multiple items. the point is OC does not serve my needs

i have no experience with this so this may look silly or not, but i tried today implementing a custom type which inherits from List and INotifyCollectionChanged. i used "public new void" keywords to hide the inherited member and make it use my versions, which really just base version and then raising the CollectionChanged event so that the CollectionView refreshes. i did this for Add and AddRange. works perfectly.

as for Clear(), raising the event is not necessary but i noticed that calling Clear() did not do anything to keep memory usage down. what fixed the memory leak issue for me here is to just call

    public new void Clear()
    {
        base.Clear();
        GC.Collect(2);
    }

from doing a lot of reading ive been discouraged from forcing a GC.Collect() manually. but here i do it anyway. the result being that now my CollectionView, despite constantly removing and adding items to my custom type as mentioned above, is behaving as i want it to. memory usage only climbs up to a certain point (a few MB) where it then stops increasing. well it's not perfect but it's night and day compared to before

i hope this helps others. i have no idea if what i did above is good coding practice (most likely not) but no more (obscene) memory leak. ive been struggling with this issue now for months and was convinced i had to retire my application but now i can continue

thank you .NET MAUI team for your hard work, this framework is really nice though it definitely has some issues but for my relatively simple application that i need for some house tasks it is fine for now, i hope everything works out

@feal87
Copy link

feal87 commented Jan 7, 2023

Adding/removing items from the collection isn't the problem of this ticket (that is, in fact, the workaround). The problem is related to the way MAUI uses the components underneath (WinUI) to render the collection if the binding changes (you assign a new one).
As long as you stay with a single observablecollection and keep refreshing it it's fine and it doesn't break.

Also you don't need to do a whole new class to have an AddRange. Just inherit from ObservableCollection and add the method

        public void AddRange(IEnumerable<T> items)
        {
            this.CheckReentrancy();
            foreach (var item in items)
                this.Items.Add(item);
            this.OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
        }

Like this and you're done without triggering multiple events.

@hartez hartez added the area-controls-listview ListView and TableView label Jan 13, 2023
@hartez hartez added area-controls-collectionview CollectionView, CarouselView, IndicatorView and removed investigate labels Jan 13, 2023
@symbiogenesis
Copy link
Contributor

Adding/removing items from the collection isn't the problem of this ticket (that is, in fact, the workaround). The problem is related to the way MAUI uses the components underneath (WinUI) to render the collection if the binding changes (you assign a new one). As long as you stay with a single observablecollection and keep refreshing it it's fine and it doesn't break.

Also you don't need to do a whole new class to have an AddRange. Just inherit from ObservableCollection and add the method

        public void AddRange(IEnumerable<T> items)
        {
            this.CheckReentrancy();
            foreach (var item in items)
                this.Items.Add(item);
            this.OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
        }

Like this and you're done without triggering multiple events.

I like your code. I've been using something similar that I found on StackOverflow which does the same thing via extensions without needing to inherit.

public static class ObservableCollectionExtensions
{
    public static void AddRange<T>(this ObservableCollection<T> collection, IEnumerable<T> items)
    {
        if (collection is null || items?.Any() != true)
        {
            return;
        }

        var type = collection.GetType();

        if (type.BaseType is null)
        {
            return;
        }

        _ = type.InvokeMember("CheckReentrancy", BindingFlags.Instance | BindingFlags.InvokeMethod | BindingFlags.NonPublic, null, collection, null, CultureInfo.InvariantCulture);

        var itemsProp = type.BaseType.GetProperty("Items", BindingFlags.NonPublic | BindingFlags.FlattenHierarchy | BindingFlags.Instance);

        if (itemsProp == null)
        {
            return;
        }

        var privateItems = (IList<T>)(itemsProp.GetValue(collection) ?? throw new FieldAccessException("list has no Items"));
        foreach (var item in items)
        {
            privateItems.Add(item);
        }

        _ = type.InvokeMember(
            "OnPropertyChanged",
            BindingFlags.Instance | BindingFlags.InvokeMethod | BindingFlags.NonPublic,
            null,
            collection,
            new object[] { new PropertyChangedEventArgs("Count") },
            CultureInfo.InvariantCulture);

        _ = type.InvokeMember(
            "OnPropertyChanged",
            BindingFlags.Instance | BindingFlags.InvokeMethod | BindingFlags.NonPublic,
            null,
            collection,
            new object[] { new PropertyChangedEventArgs("Item[]") },
            CultureInfo.InvariantCulture);

        _ = type.InvokeMember(
            "OnCollectionChanged",
            BindingFlags.Instance | BindingFlags.InvokeMethod | BindingFlags.NonPublic,
            null,
            collection,
            new object[] { new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, newItems: items.ToList(), oldItems: Array.Empty<T>()) },
            CultureInfo.InvariantCulture);
    }

    public static void RemoveRange<T>(this ObservableCollection<T> collection, IEnumerable<T> items)
    {
        if (collection is null || items?.Any() != true)
        {
            return;
        }

        var type = collection.GetType();

        if (type.BaseType is null)
        {
            return;
        }

        _ = type.InvokeMember("CheckReentrancy", BindingFlags.Instance | BindingFlags.InvokeMethod | BindingFlags.NonPublic, null, collection, null, CultureInfo.InvariantCulture);

        var itemsProp = type.BaseType.GetProperty("Items", BindingFlags.NonPublic | BindingFlags.FlattenHierarchy | BindingFlags.Instance);

        if (itemsProp == null)
        {
            return;
        }

        var privateItems = (IList<T>)(itemsProp.GetValue(collection) ?? throw new FieldAccessException("list has no Items"));
        foreach (var item in items)
        {
            _ = privateItems.Remove(item);
        }

        _ = type.InvokeMember(
            "OnPropertyChanged",
            BindingFlags.Instance | BindingFlags.InvokeMethod | BindingFlags.NonPublic,
            null,
            collection,
            new object[] { new PropertyChangedEventArgs("Count") },
            CultureInfo.InvariantCulture);

        _ = type.InvokeMember(
            "OnPropertyChanged",
            BindingFlags.Instance | BindingFlags.InvokeMethod | BindingFlags.NonPublic,
            null,
            collection,
            new object[] { new PropertyChangedEventArgs("Item[]") },
            CultureInfo.InvariantCulture);

        _ = type.InvokeMember(
            "OnCollectionChanged",
            BindingFlags.Instance | BindingFlags.InvokeMethod | BindingFlags.NonPublic,
            null,
            collection,
            new object[] { new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, newItems: Array.Empty<T>(), oldItems: items.ToList()) },
            CultureInfo.InvariantCulture);
    }
}

@DamianSuess
Copy link

DamianSuess commented Feb 19, 2023

This issue does happen with Inserting and Removing items from the ListView (see screenshot below).

Unfortunately, this cripples our application where we are presenting a live data stream of the most recent (100) items. When the flow is coming in every 100-50ms, it takes only a mere 30 seconds for the application to become unusable.

The simple example below inserts an item to the top of the ListView and removes the bottom-most item from the list. As you can see from the screenshot, there is a strong amount of GC pressure and it never goes down. Even after stopping this test loop (not pictured), the memory is still spiked.

If you want this as a separate issue, let me know and I'll report it.

  public ObservableCollection<string> PacketCache
  {
    get => _packetCache;
    set => SetProperty(ref _packetCache, value);
  }

  public DelegateCommand<string> CmdTest => new((string _) =>
  {
    int x = 0;
    for (;;)
    {
      PacketCache.Insert(0, x.ToString());
      if (PacketCache.Count >= 100)
        PacketCache.RemoveAt(99);
      x++;
    }
  });
    <ListView ItemsSource="{Binding PacketCache}"
              SelectedItem="{Binding PacketSelected}"
              SelectionMode="Single">
      <ListView.ItemTemplate>
        <DataTemplate x:DataType="x:String">
          <ViewCell>
            <Label Text="{Binding .}" FontFamily="Consolas" />
          </ViewCell>
        </DataTemplate>
      </ListView.ItemTemplate>
    </ListView>

image

@samhouts samhouts added the p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint label Feb 22, 2023
@samhouts samhouts modified the milestones: Backlog, .NET 8 Planning Feb 22, 2023
@jonathanpeppers jonathanpeppers self-assigned this Apr 18, 2023
@jonathanpeppers
Copy link
Member

jonathanpeppers commented Apr 18, 2023

At first, I thought this repro might be fixed by these:

But it is it is nearly fixed. It appears to leak around 28 objects as you navigate to the new page and back.

I found if I commented out the code in two pages:

ToolbarItems.Add(new ToolbarItem("Refresh", "ic_refresh.png", () => {
    listView.ItemsSource = GenerateDisplayItems();
}));

Then it is completely solved as memory goes up and down a small amount:

image

I will investigate further, as it points to a potential memory leak in Page.ToolbarItems.

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Apr 19, 2023

After further investigation for this sample today:

  1. Tested latest dotnet/maui/main.
  2. I put GC.Collect(); GC.WaitForPendingFinalizers(); in the sample.
  3. Tested a Release build of the app on Windows.

I believe the problems here are now fixed in dotnet/maui/main. Likely #13260 and #14329 specifically.

I can see the memory go up by a very small amount, and then goes down:

image

When it goes down, it appears to hit the threshold established in WeakList<T> in #13260.

I also tried adding ToolbarItem in this unit test (which passes successfully):

[Fact(DisplayName = "NavigationPage Does Not Leak")]
public async Task DoesNotLeak()

I believe this problem is solved, but I'm always willing to try other sample projects to see if I find anything wrong. You should be able to test these changes in upcoming .NET 8 previews.

Thanks!

@DamianSuess
Copy link

@jonathanpeppers Thank you, sir! Always appreciate you taking care of Xamarin/MAUI and providing your magical touch of precision 👍

@ghost ghost locked as resolved and limited conversation to collaborators May 20, 2023
@samhouts samhouts added the fixed-in-7.0.92 Look for this fix in 7.0.92! label Jul 11, 2023
@samhouts samhouts added fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! fixed-in-7.0.81 Look for this fix in 7.0.81! labels Jul 20, 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 area-controls-listview ListView and TableView 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.1.7762 Look for this fix in 8.0.0-preview.1.7762! p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint 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.