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.

Sign up for GitHub

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

Jump to bottom

Memory leak with CarouselView #17726

Closed
tranb3r opened this issue Sep 28, 2023 · 6 comments · Fixed by #18584
Closed

Memory leak with CarouselView #17726

tranb3r opened this issue Sep 28, 2023 · 6 comments · Fixed by #18584
Assignees
@jonathanpeppers
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView fixed-in-8.0.6 Look for this fix in 8.0.6 SR1! memory-leak 💦 Memory usage grows / objects live forever (sub: perf) platform/android 🤖 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Milestone
.NET 8 SR1

Comments

@tranb3r
Copy link

tranb3r commented Sep 28, 2023

Description

On android with net8-rc.1, when navigating to a page containing a CarouselView and then back, the memory usage grows.
For comparison, no memory leak when navigating to a page containing a CollectionView with the same ItemsSource and ItemTemplate.

Steps to Reproduce

  1. Open repro project
  2. On the first tab, tap on Go button, it will navigate to a page with a CollectionView.
  3. Go back to the main page. Repeat a few times. Observe memory usage with dotnet-gcdump. It is stable.
  4. On the second tab, tap on Go button, it will navigate to a page with a CarouselView.
  5. Go back to the main page. Repeat a few times. Observe memory usage with dotnet-gcdump. It is slowly increasing.
  6. Alternatively, open the attached gcdump files I've captured when showing the CarouselView page a few times. Between the first (memory-20.gcdump) and second (memory-24.gcdump), the memory usage increases quite a lot (the page is really simple).

gcdump.zip

Screenshot 2023-09-28 172824

Link to public reproduction project repository

https://github.com/tranb3r/Issues/tree/main/MauiAppCarouselViewLeak

Version with bug

8.0.0-rc.1.9171

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

Android

Affected platform versions

Android 34

Did you find any workaround?

No workaround

Relevant log output

No response
@tranb3r tranb3r added the t/bug Something isn't working label Sep 28, 2023
@jsuarezruiz jsuarezruiz added legacy-area-perf Startup / Runtime performance area-controls-collectionview CollectionView, CarouselView, IndicatorView platform/android 🤖 labels Sep 29, 2023
@jsuarezruiz
Copy link
Contributor

cc @jonathanpeppers

@jonathanpeppers
Copy link
Member

@tranb3r I compared your two .gcdump files above and I only see:

image

It will probably take a Java side GC + .NET GC for these IdentityHashTarget objects to go away, this should eventually be called:

https://github.com/xamarin/xamarin-android/blob/0945978452a357f0dd8eb89d9c1bca523bbe0e5c/src/Mono.Android/Android.Runtime/AndroidRuntime.cs#L792

If you do something like this, are the MAUI Page objects going away in your sample?

image

I was hoping to see more than one Page2 object in your .gcdump files, but it only has 1. If you somehow see many Page2's living forever, that 100% seems like it would be a leak in MAUI.

@nmanocha
Copy link

This problem is reproducible with iOS also with net8-rc.1 and net8-rc.2

@jonathanpeppers
Copy link
Member

I think the issue with @tranb3r's dumps might be related to:

dotnet/diagnostics#4081 (comment)

I think the finalizer isn't running for Page2, but then the Mono runtime + diagnostic tooling isn't showing these objects in the .gcdump either.

I will try .NET 7 & latest dotnet/maui/main, maybe I can get the dumps using the old method in .NET 7.

@jonathanpeppers jonathanpeppers added the memory-leak 💦 Memory usage grows / objects live forever (sub: perf) label Oct 13, 2023
@jonathanpeppers
Copy link
Member

Seems like .NET 7 also only shows a single Page2:

image

But I never see a log message from ~Page2() => Console.WriteLine("~Page2");.

@lateralusX lateralusX mentioned this issue Oct 16, 2023
Merged
@jonathanpeppers jonathanpeppers self-assigned this Oct 20, 2023
@jonathanpeppers jonathanpeppers added this to the .NET 8 + Servicing milestone Oct 20, 2023
@jonathanpeppers
Copy link
Member

I made some progress on this here, could reproduce the issue in tests:

main...jonathanpeppers:maui:CarouselViewLeaks

There is an issue in ObservableItemsSource, which is used internally by all ItemsView types like CarouselView.

But there is still a further leak, I'll need to investigate more next week. My two tests here that put CarouselView in a NavigationPage and TabbedPage both still fail on Android.

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Oct 23, 2023
@jonathanpeppers
[core] fix memory leak with CarouselView & INotifyCollectionChanged
c3c996b 
Context: dotnet#17726

In investigating dotnet#17726, I found a memory leak with `CarouselView`:

1. Have a long-lived `INotifyCollectionChanged` like
   `ObservableCollection`, that lives the life of the application.

2. Set `CarouselView.ItemsSource` to the collection.

3. `CarouselView` will live forever, even if the page is popped, etc.

I further expanded upon `MemoryTests` to assert more things for each
control, and was able to reproduce this issue.

To solve the problem, I used the same technique in 58a42e5. We can use
`WeakNotifyCollectionChangedProxy` in the `ObservableItemsSource` class
to prevent `CarouselView` from leaking. `ObservableItemsSource` is used
for other controls, so this may fix other leaks as well.

Unfortunately, this does not fully solve dotnet#17726, as there is at least
one further leak on Android from my testing.
@jonathanpeppers jonathanpeppers mentioned this issue Oct 23, 2023
Merged
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Oct 24, 2023
@jonathanpeppers
[controls] fix memory leak with CarouselView & `INotifyCollectionCh…
b1d8176 
…anged`

Context: dotnet#17726

In investigating dotnet#17726, I found a memory leak with `CarouselView`:

1. Have a long-lived `INotifyCollectionChanged` like
   `ObservableCollection`, that lives the life of the application.

2. Set `CarouselView.ItemsSource` to the collection.

3. `CarouselView` will live forever, even if the page is popped, etc.

I further expanded upon `MemoryTests` to assert more things for each
control, and was able to reproduce this issue.

To fully solve this, I had to fix issues on each platform.

~~ Android ~~

`ObservableItemsSource` subscribes to the `INotifyCollectionChanged`
event, making it live forever in this case.

To fix this, I used the same technique in 58a42e5:
`WeakNotifyCollectionChangedProxy`. `ObservableItemsSource` is used for
`ItemsView`-like controls, so this may fix other leaks as well.

~~ Windows ~~

Windows has the same issue as Android, but for the following types:

* `CarouselViewHandler` subscribes to `INotifyCollectionChanged`

* `ObservableItemTemplateCollection` subscribes to
  `INotifyCollectionChanged`

These could be fixed with `WeakNotifyCollectionChangedProxy`.

Then I found another issue with `ItemTemplateContext`

These three types are used for other `ItemsView`-like controls, so this
may fix other leaks as well.

~~ iOS/Catalyst ~~

These platforms suffered from multiple circular references:

* `CarouselViewController` -> `CarouselView` -> `CarouselViewHandler` ->
  `CarouselViewController`

* `ItemsViewController` -> `CarouselView` -> `CarouselViewHandler` ->
  `ItemsViewController` (`CarouselViewController` subclasses this)

* `CarouselViewLayout` -> `CarouselView` -> `CarouselViewHandler` ->
  `CarouselViewLayout`

The analyzer added in 2e65626 did not yet catch these because I only
added the analyzer so far for `Microsoft.Maui.dll` and these issues were
in `Microsoft.Maui.Controls.dll`. In a future PR, we can proactively try
to add the analyzer to all projects.

~~ Conclusion ~~

Unfortunately, this does not fully solve dotnet#17726, as there is at least
one further leak on Android from my testing. But this is a good step
forward.
@jonathanpeppers jonathanpeppers moved this to In Progress in MAUI SDK Ongoing Oct 30, 2023
jonathanpeppers added a commit that referenced this issue Nov 7, 2023
@jonathanpeppers
[controls] fix memory leak with CarouselView & `INotifyCollectionCh…
6f073d6 
…anged` (#18267)

Context: #17726

In investigating #17726, I found a memory leak with `CarouselView`:

1. Have a long-lived `INotifyCollectionChanged` like
   `ObservableCollection`, that lives the life of the application.

2. Set `CarouselView.ItemsSource` to the collection.

3. `CarouselView` will live forever, even if the page is popped, etc.

I further expanded upon `MemoryTests` to assert more things for each
control, and was able to reproduce this issue.

To fully solve this, I had to fix issues on each platform.

~~ Android ~~

`ObservableItemsSource` subscribes to the `INotifyCollectionChanged`
event, making it live forever in this case.

To fix this, I used the same technique in 58a42e5:
`WeakNotifyCollectionChangedProxy`. `ObservableItemsSource` is used for
`ItemsView`-like controls, so this may fix other leaks as well.

~~ Windows ~~

Windows has the same issue as Android, but for the following types:

* `CarouselViewHandler` subscribes to `INotifyCollectionChanged`

* `ObservableItemTemplateCollection` subscribes to
  `INotifyCollectionChanged`

These could be fixed with `WeakNotifyCollectionChangedProxy`.

Then I found another issue with `ItemTemplateContext`

These three types are used for other `ItemsView`-like controls, so this
may fix other leaks as well.

~~ iOS/Catalyst ~~

These platforms suffered from multiple circular references:

* `CarouselViewController` -> `CarouselView` -> `CarouselViewHandler` ->
  `CarouselViewController`

* `ItemsViewController` -> `CarouselView` -> `CarouselViewHandler` ->
  `ItemsViewController` (`CarouselViewController` subclasses this)

* `CarouselViewLayout` -> `CarouselView` -> `CarouselViewHandler` ->
  `CarouselViewLayout`

The analyzer added in 2e65626 did not yet catch these because I only
added the analyzer so far for `Microsoft.Maui.dll` and these issues were
in `Microsoft.Maui.Controls.dll`. In a future PR, we can proactively try
to add the analyzer to all projects.

~~ Conclusion ~~

Unfortunately, this does not fully solve #17726, as there is at least
one further leak on Android from my testing. But this is a good step
forward.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Nov 7, 2023
@jonathanpeppers
[android] fix memory leak with CarouselView
d083d09 
Fixes: dotnet#17726

Adding `CarouselView` to a page and navigating away from it keeps the
page alive forever on Android. I was able to reproduce this in a test.

It took me a while to actually track this one down, but the problem
boils down to `MauiCarouselRecyclerView` doing:

    _carouselViewLayoutListener = new CarouselViewwOnGlobalLayoutListener();
    _carouselViewLayoutListener.LayoutReady += LayoutReady;
    ViewTreeObserver.AddOnGlobalLayoutListener(_carouselViewLayoutListener);

The `ViewTreeObserver` lives longer than the page, and so:

* `ViewTreeObserver` -> `CarouselViewwOnGlobalLayoutListener`
* `event LayoutReady` -> `MauiCarouselRecyclerView`
* `MauiCarouselRecyclerView` -> `CarouselView`
* `CarouselView` -> `Parent.Parent.Parent.*` -> `Page`

Thus the `Page` lives forever.

If we remove the `LayoutReady` event, instead:

* `CarouselViewwOnGlobalLayoutListener` saves `MauiCarouselRecyclerView`
in a weak reference.

* It can just call `LayoutReady()` directly.

* `MauiCarouselRecyclerView` is able to be GC'd now.

* `MauiCarouselRecyclerView.Dispose()` already appropriately will call
  `ClearLayoutListener()`

* Lastly, `ViewTreeObserver?.RemoveOnGlobalLayoutListener(_carouselViewLayoutListener)`

With these changes, the test passes and I don't think anything will leak
now. :)

Other changes:

* I fixed the typo in `CarouselViewwOnGlobalLayoutListener`.
* I sorted the type names in the test alphabetically.
@jonathanpeppers jonathanpeppers mentioned this issue Nov 7, 2023
Merged
rmarinho pushed a commit that referenced this issue Nov 8, 2023
@jonathanpeppers
[android] fix memory leak with CarouselView (#18584)
5d71344 
Fixes: #17726

Adding `CarouselView` to a page and navigating away from it keeps the
page alive forever on Android. I was able to reproduce this in a test.

It took me a while to actually track this one down, but the problem
boils down to `MauiCarouselRecyclerView` doing:

    _carouselViewLayoutListener = new CarouselViewwOnGlobalLayoutListener();
    _carouselViewLayoutListener.LayoutReady += LayoutReady;
    ViewTreeObserver.AddOnGlobalLayoutListener(_carouselViewLayoutListener);

The `ViewTreeObserver` lives longer than the page, and so:

* `ViewTreeObserver` -> `CarouselViewwOnGlobalLayoutListener`
* `event LayoutReady` -> `MauiCarouselRecyclerView`
* `MauiCarouselRecyclerView` -> `CarouselView`
* `CarouselView` -> `Parent.Parent.Parent.*` -> `Page`

Thus the `Page` lives forever.

If we remove the `LayoutReady` event, instead:

* `CarouselViewwOnGlobalLayoutListener` saves `MauiCarouselRecyclerView`
in a weak reference.

* It can just call `LayoutReady()` directly.

* `MauiCarouselRecyclerView` is able to be GC'd now.

* `MauiCarouselRecyclerView.Dispose()` already appropriately will call
  `ClearLayoutListener()`

* Lastly, `ViewTreeObserver?.RemoveOnGlobalLayoutListener(_carouselViewLayoutListener)`

With these changes, the test passes and I don't think anything will leak
now. :)

Other changes:

* I fixed the typo in `CarouselViewwOnGlobalLayoutListener`.
* I sorted the type names in the test alphabetically.
@github-project-automation github-project-automation bot moved this from In Progress to Done in MAUI SDK Ongoing Nov 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
@samhouts samhouts added the fixed-in-8.0.6 Look for this fix in 8.0.6 SR1! label Jan 31, 2024
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) 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.
Assignees

@jonathanpeppers jonathanpeppers

Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView fixed-in-8.0.6 Look for this fix in 8.0.6 SR1! memory-leak 💦 Memory usage grows / objects live forever (sub: perf) platform/android 🤖 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
MAUI SDK Ongoing
Status: Done
Milestone
.NET 8 SR1
Development

Successfully merging a pull request may close this issue.

[android] fix memory leak with CarouselView jonathanpeppers/maui
6 participants
@Eilon @jonathanpeppers @jsuarezruiz @samhouts @nmanocha @tranb3r