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

Memory leaks on latest .NET 8 preview with Windows #16436

Open
symbiogenesis opened this issue Jul 29, 2023 · 25 comments
Open

Memory leaks on latest .NET 8 preview with Windows #16436

symbiogenesis opened this issue Jul 29, 2023 · 25 comments
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView memory-leak 💦 Memory usage grows / objects live forever migration-compatibility Xamarin.Forms to .NET MAUI Migration, Upgrade Assistant, Try-Convert platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Milestone

Comments

@symbiogenesis
Copy link
Contributor

symbiogenesis commented Jul 29, 2023

Description

A data grid that I am working on leaks memory massively as you scroll around when there are a lot of records.

Steps to Reproduce

Just run the sample I provided, and start scrolling, etc.

Link to public reproduction project repository

https://github.com/symbiogenesis/Maui.DataGrid/tree/net8-memory-leak

Version with bug

8.0.0-preview.6.8686

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

All

Did you find any workaround?

No.

Relevant log output

No response

@symbiogenesis symbiogenesis added the t/bug Something isn't working label Jul 29, 2023
@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Jul 29, 2023

@jonathanpeppers You may have some thoughts. Manually running GC doesn't help.

@jsuarezruiz jsuarezruiz added platform/windows 🪟 legacy-area-perf Startup / Runtime performance area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Jul 31, 2023
@samhouts samhouts added the memory-leak 💦 Memory usage grows / objects live forever label Aug 1, 2023
@jonathanpeppers
Copy link
Member

Everywhere you see += with an event in this file is a potential leak:

https://github.com/symbiogenesis/Maui.DataGrid/blob/net8-memory-leak/Maui.DataGrid/DataGrid.xaml.cs

So far looking at this, I see several potential leaks in this DataGrid control. It's probably not worth looking if there are further MAUI issues here without those solved.

Have you seen this wiki page? It might help you diagnose and track these down:

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Aug 5, 2023

My code is already using the WeakEventManager for all of my events, and I unsubscribe to all of the events at what I think is the proper times, with the exception of a minor change I just made.

Not sure what else I ought to be doing. That link shows some similar event usage, and links to a PR that implements the WeakEventManager approach that I'm already using.

@symbiogenesis
Copy link
Contributor Author

I think I see now. So the subscriptions to events that are located outside of MAUI in the .NET Framework are unsafe, and need a proxy wrapper to make them safe.

I will implement that and try again.

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Aug 7, 2023

I just implemented that in the linked branch, except where WeakEventManager was being used, and the memory leaks remain

@jonathanpeppers
Copy link
Member

Especially on Windows, you should be able to see a tree of objects from a memory snapshot. What object is holding on to another object? If you can share their fully qualified name or the .gcdump file that would make things clearer.

In addition to the Narrowing down the leak instructions:

https://github.com/dotnet/maui/wiki/Memory-Leaks#narrowing-down-the-leak

Does the problem go away if you swap your custom DataGrid control with something built-in? Like a ListView or CollectionView?

@symbiogenesis
Copy link
Contributor Author

image

image

@jonathanpeppers
Copy link
Member

Can you address the XAML hot reload warning in your screenshot?

We mention it on the wiki page:

image

Did you also sprinkle some GC.Collect() calls in the app as mentioned?

image

@symbiogenesis
Copy link
Contributor Author

I had tried using GC.Collect() in the app before, to no effect. Just tried now, and there was no effect.

Just tried turning off XAML Hot Reload, and it had no effect.

Also removed the more complicated columns and just kept some simple text columns. And the leak remains.

image

@jonathanpeppers
Copy link
Member

Can you comment on:

Does the problem go away if you swap your custom DataGrid control with something built-in? Like a ListView or CollectionView?

Your latest screenshot shows the DataGrid, but what is the exact object you think is leaking? So far, what I see are problems in this DataGrid control.

@symbiogenesis
Copy link
Contributor Author

The DataGrid control is mainly a wrapper for the CollectionView. Each row is a Grid, with identical ColumnDefinitions.

I personally think that the CollectionView is leaking.

@symbiogenesis
Copy link
Contributor Author

Changing the CollectionView to a ListView does not resolve the problem.

@jonathanpeppers
Copy link
Member

Can you share a branch of your sample that uses ListView or CollectionView?

https://github.com/symbiogenesis/Maui.DataGrid/tree/net8-memory-leak

It isn't using the DataGrid at all, correct?

@symbiogenesis
Copy link
Contributor Author

I just updated this branch to just use a simple CollectionView, and the ListView version is commented out below it.

With this simple version, it also leaks, yes.

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Aug 7, 2023

Specifically, I think it is only when using an ItemTemplate, and this issue has existed since Xamarin.Forms

@jonathanpeppers
Copy link
Member

I found this issue with ListView: #16450 (comment)

But let me see if something similar is happening with CollectionView as that seems more important. (It's the new control you're supposed to use)

@symbiogenesis
Copy link
Contributor Author

This might have more to do with the Grid being inside of the ItemTemplate, rather than the ItemTemplate in general.

@jonathanpeppers
Copy link
Member

@symbiogenesis I did some debugging, and it appears the way the CollectionView works.

  1. You have 150,000 rows, it apparently creates a List<ItemTemplateContext> with 150,000 null items...
  2. This code is called as each row is scrolled into view:

if (_itemTemplateContexts[index] == null)
{
_itemTemplateContexts[index] = new ItemTemplateContext(_itemTemplate, _itemsSource[index],
_container, _itemHeight, _itemWidth, _itemSpacing, _mauiContext);
}

And so ItemTemplateContext instances will grow as you scroll each one into view. I did not see the memory growing when I scroll to rows that were already on screen:

image

Now, I do see some things in the code here that could be improved -- but I'm not seeing a leak so far.

Do you feel like you are seeing something different than me? I will see if ListView feels any different.

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Aug 17, 2023

One reason that I think it leaks is that navigating away from the page doesn't free up any memory. Even with a manual garbage collect.

I also tend to use more complicated DataTemplates, and with these scrolling back to rows that were already loaded does increase the memory usage.

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Aug 17, 2023
Context: dotnet#16436
Context: https://github.com/symbiogenesis/Maui.DataGrid/tree/net8-memory-leak

A customer sample has a `CollectionView` with 150,000 data-bound rows.

Debugging what happens at runtime, I found we effectively were doing:

    _itemTemplateContexts = new List<ItemTemplateContext>(capacity: 150_000);
    for (int n = 0; n < 150_000; n++)
    {
        _itemTemplateContexts.Add(null);
    }

Then the items were created as you scroll each row into view:

    if (_itemTemplateContexts[index] == null)
    {
        _itemTemplateContexts[index] = context = new ItemTemplateContext(_itemTemplate, _itemsSource[index],
            _container, _itemHeight, _itemWidth, _itemSpacing, _mauiContext);
    }
    return _itemTemplateContexts[index];

This code accesses the indexer multiple times, into a `List<T>` with
150,000 `null` items.

To improve this:

* use a `Dictionary<int, T>` instead, just let it size dynamically.

* use `TryGetValue(..., out var context)`, so each call accesses the
  indexer one less time than before.

* use either the bound collection's size or 64 (whichever is smaller) as
  a rough estimate of how many might fit on screen at a time.

Taking a memory snapshot of the app after startup:

    Before:
    Heap Size: 82,899.54 KB
    After:
    Heap Size: 81,768.76 KB

Which is saving about 1MB of memory on launch.

In this case, it feels better to just let the `Dictionary` size itself
with an estimate of what `capacity` will be.
@jonathanpeppers
Copy link
Member

navigating away from the page doesn't free up any memory
I also tend to use more complicated DataTemplates

Can you update the sample to show this behavior? Or some examples of DataTemplate that leak?

I tried ListView, but it also seems OK when this change (merged today) is in place: 040f2f1

image

I opened this one, as an obvious improvement looking at the existing code:

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Aug 17, 2023
Context: dotnet#16436
Context: https://github.com/symbiogenesis/Maui.DataGrid/tree/net8-memory-leak

A customer sample has a `CollectionView` with 150,000 data-bound rows.

Debugging what happens at runtime, I found we effectively were doing:

    _itemTemplateContexts = new List<ItemTemplateContext>(capacity: 150_000);
    for (int n = 0; n < 150_000; n++)
    {
        _itemTemplateContexts.Add(null);
    }

Then the items were created as you scroll each row into view:

    if (_itemTemplateContexts[index] == null)
    {
        _itemTemplateContexts[index] = context = new ItemTemplateContext(_itemTemplate, _itemsSource[index],
            _container, _itemHeight, _itemWidth, _itemSpacing, _mauiContext);
    }
    return _itemTemplateContexts[index];

This code accesses the indexer multiple times, into a `List<T>` with
150,000 `null` items.

To improve this:

* use a `Dictionary<int, T>` instead, just let it size dynamically.

* use `TryGetValue(..., out var context)`, so each call accesses the
  indexer one less time than before.

* use either the bound collection's size or 64 (whichever is smaller) as
  a rough estimate of how many might fit on screen at a time.

Taking a memory snapshot of the app after startup:

    Before:
    Heap Size: 82,899.54 KB
    After:
    Heap Size: 81,768.76 KB

Which is saving about 1MB of memory on launch.

In this case, it feels better to just let the `Dictionary` size itself
with an estimate of what `capacity` will be.
rmarinho pushed a commit that referenced this issue Aug 21, 2023
Context: #16436
Context: https://github.com/symbiogenesis/Maui.DataGrid/tree/net8-memory-leak

A customer sample has a `CollectionView` with 150,000 data-bound rows.

Debugging what happens at runtime, I found we effectively were doing:

    _itemTemplateContexts = new List<ItemTemplateContext>(capacity: 150_000);
    for (int n = 0; n < 150_000; n++)
    {
        _itemTemplateContexts.Add(null);
    }

Then the items were created as you scroll each row into view:

    if (_itemTemplateContexts[index] == null)
    {
        _itemTemplateContexts[index] = context = new ItemTemplateContext(_itemTemplate, _itemsSource[index],
            _container, _itemHeight, _itemWidth, _itemSpacing, _mauiContext);
    }
    return _itemTemplateContexts[index];

This code accesses the indexer multiple times, into a `List<T>` with
150,000 `null` items.

To improve this:

* use a `Dictionary<int, T>` instead, just let it size dynamically.

* use `TryGetValue(..., out var context)`, so each call accesses the
  indexer one less time than before.

* use either the bound collection's size or 64 (whichever is smaller) as
  a rough estimate of how many might fit on screen at a time.

Taking a memory snapshot of the app after startup:

    Before:
    Heap Size: 82,899.54 KB
    After:
    Heap Size: 81,768.76 KB

Which is saving about 1MB of memory on launch.

In this case, it feels better to just let the `Dictionary` size itself
with an estimate of what `capacity` will be.
@rachelkang rachelkang added the migration-compatibility Xamarin.Forms to .NET MAUI Migration, Upgrade Assistant, Try-Convert label Jan 23, 2024
@scriptBoris
Copy link

I created a simple project, specified this XAML and using mouse scrolling up and down, you can increase memory consumption to over 3 gigabytes

<?xml version="1.0" encoding="utf-8" ?>
<ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
             xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
             x:Class="Sample.MultipleButtonsPage"
             Title="test">
    <StackLayout>
        <CollectionView x:Name="collectionView"
                        ItemSizingStrategy="MeasureFirstItem"
                        VerticalOptions="FillAndExpand">
            <CollectionView.ItemTemplate>
                <DataTemplate>
                    <ContentView Padding="10">
                        <Label Text="{Binding .}"/>
                    </ContentView>
                </DataTemplate>
            </CollectionView.ItemTemplate>
        </CollectionView>
    </StackLayout>
</ContentPage>

(ItemsSource is simple List with 1000 elements)
LOL

@scriptBoris
Copy link

NET8, Controls nuget lib version 8.0.6

@symbiogenesis
Copy link
Contributor Author

@scriptBoris I see the same thing as you. I just submitted PR #20036 that may help a bit with CollectionView on Windows. I'm sure there's lots more to do.

@Zhanglirong-Winnie Zhanglirong-Winnie added s/verified Verified / Reproducible Issue ready for Engineering Triage s/triaged Issue has been reviewed labels Feb 20, 2024
@Zhanglirong-Winnie
Copy link
Collaborator

Verified this issue with Visual Studio Enterprise 17.10.0 Preview 1. Can repro on Windows platform with sample project. I disabled XAML Hot Reload, and after successful debugging, I scrolled up and down, but the problem still occurred.
https://github.com/symbiogenesis/Maui.DataGrid/tree/net8-memory-leak
image

@kaushik-sarker
Copy link

Here is a sample where you can reproduce the issue. Run the application and click on the "Reload Data" button. On every data load, you will see that memory consumption is increasing. The sample contains a simple data template with a grid.

@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
@jsuarezruiz jsuarezruiz added this to the Backlog milestone Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView memory-leak 💦 Memory usage grows / objects live forever migration-compatibility Xamarin.Forms to .NET MAUI Migration, Upgrade Assistant, Try-Convert platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

No branches or pull requests

9 participants