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] improve memory usage of CollectionView #16838

Merged
merged 1 commit into from Aug 21, 2023

Conversation

jonathanpeppers
Copy link
Member

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.

@Eilon Eilon added the area/collectionview 📃 CollectionView, CarouselView, IndicatorView label Aug 17, 2023
@symbiogenesis
Copy link
Contributor

You said

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.

The code uses Math.Max. Based on your comment, perhaps Math.Min would be the right answer here?

@samhouts samhouts added the memory-leak 💦 Memory usage grows / objects live forever label Aug 17, 2023
@samhouts samhouts added this to the .NET 8 GA milestone 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 Author

Yes, I tested w/ no capacity and just put the wrong one... Thanks.

@samhouts
Copy link
Member

Also related? #15523

@jonathanpeppers jonathanpeppers marked this pull request as ready for review August 18, 2023 14:14
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner August 18, 2023 14:14
@rmarinho
Copy link
Member

Also related? #15523

I don't think so, this doesn't really fix a leak, but just avoids create a large list with all the items (null) from the start. Saves memory.

Seems we still need a better repo to show the "leak" on #16436 , but yeah the report is similar to #15523.

@rmarinho
Copy link
Member

We can also ignore the UITests are failing.

@rmarinho rmarinho merged commit 9fea1f5 into dotnet:main Aug 21, 2023
34 of 39 checks passed
@jonathanpeppers jonathanpeppers deleted the ItemTemplateContextList branch August 21, 2023 15:24
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/collectionview 📃 CollectionView, CarouselView, IndicatorView memory-leak 💦 Memory usage grows / objects live forever platform/windows 🪟
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants