Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Fixes various memory leaks in listview with group headers (#6761) #12535

Closed
wants to merge 12 commits into from

Conversation

t-johnson
Copy link
Contributor

Description of Change

Fixes several places where ListView was leaking memory on IOS (and other platforms in some cases) when grouping was enabled with either a data templated defined with GroupHeaderTemplate , or a simple one set by the GroupDisplayBinding.

Issues Resolved

API Changes

None

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Android
  • UWP

(Most of the code changes are is in the IOS platform projects, however there is a small change in Xamarin.Forms.Core TemplatedItemList.css which will benefit all platforms. Note, I have only tested on IOS).

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

I've created a test page (G6761) where there are two list views with grouping enabled. One with a custom group header, and one with just the text header. These lists are bound to a collection of collections, however there are no items added in the sub-collections so the lists only render the headers. The page has a button to reload the collection, and another to force GC.
After navigating to the page, click the 'Refresh' button, and then scroll the two lists up and down a few times. use the 'GC' button to report on the current memory usage. You can click the Refresh button also to force the collections to be reset.

Before the fix, the total memory would climb as the user scrolls the lists, and also climb when the collections were reset.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@Tommigun1980
Copy link

Hi and thank you so much for these fixes!!

What release will this fix be in?

@t-johnson
Copy link
Contributor Author

@Tommigun1980 the PR is against 5.0.0. I am not sure why this is waiting so long to get merged though.

@samhouts samhouts added this to In Review in vNext+1 (5.0.0) Nov 2, 2020
@Transis-Felipe
Copy link
Contributor

Waiting for a reviewer...

@PureWeen PureWeen added this to In progress in v5.0.1 via automation Nov 5, 2020
@PureWeen PureWeen removed this from In Review in vNext+1 (5.0.0) Nov 5, 2020
@samhouts samhouts added this to In Review in vNext+1 (5.0.0) Nov 5, 2020
@samhouts samhouts removed this from In progress in v5.0.1 Nov 5, 2020
@PureWeen PureWeen added this to In progress in v5.0.1 via automation Nov 5, 2020
@PureWeen PureWeen removed this from In Review in vNext+1 (5.0.0) Nov 5, 2020
@Tommigun1980
Copy link

Hi! Any chance of getting this memory leak fix in at some point?

Thanks!

@t-johnson
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 12535 in repo xamarin/Xamarin.Forms

@t-johnson
Copy link
Contributor Author

@jsuarezruiz any chance you can trigger the pipeline for this PR also?

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jsuarezruiz
Copy link
Contributor

jsuarezruiz commented Feb 4, 2021

@t-johnson Sure. Is running now.

…eplace, TemplatedItemsListTests.UnhookGroupOnRemoval)

 - Unhook the header in the Dispose function only if it has a binding context as this is already being done in the in the case of the list remove/replace actions
@nickrandolph
Copy link
Contributor

@jsuarezruiz can you kick the build again

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AlanYost
Copy link

@t-johnson I'm not sure how we see all the PR's that you have done in one build related to memory leaks? I'm able to quickly test a build and have tested a few from the pipeline, but none have passed the memory tests. I tested 5.0.0.7127 this am with no luck.
cc. @nickrandolph

@t-johnson
Copy link
Contributor Author

@AlanYost Yeah I'm not sure how the pipeline works here, so without building it all yourself I am not sure how you can test it against your own app/sample.

Still would be nice for PRs to get either merged or rejected a whole lot faster!

@t-johnson
Copy link
Contributor Author

@StephaneDelcroix this is waiting your review?

v5.0.1 automation moved this from In progress to Reviewer approved May 3, 2021
@rachelkang
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rmarinho
Copy link
Member

rmarinho commented May 4, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@rachelkang rachelkang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t-johnson I tested on both the CG sample issue and on the original issue - unfortunately, it seems like the issue isn't fixed. What might I be missing?

On the CG - every time I click Refresh, the reported memory continues to increase.
Similarly, in the original sample - every time I click Reload, the reported memory continues to increase.

RPReplay_Final1620151327.MP4

@t-johnson
Copy link
Contributor Author

@rachelkang its been a while since I looked at this, but I believe the behavior before this fix was that even just scrolling the list up and down would leak memory so long as enough list items existed that they would go off screen. we could see this by refreshing once, then scrolling up/down and then forcing the GC.
Repeatedly clicking the 'Refresh' button in the test does seem to show the memory increasing, however by tapping the GC we can see the memory comes back down, and is somewhat stable.

@jfversluis
Copy link
Member

Now that we're so close to the sunsetting of Xamarin.Forms unfortunately we won't be able to take this in anymore, we're really sorry about that. Nevertheless, thank you so much for your time and effort that you have put into this PR.

Please have a look at the evolution of Xamarin.Forms, .NET MAUI. A lot of development has been going on there. Hopefully this issue was already fixed in that codebase. If not, feel free to port this over to there.

Again, thank you so much for being a contributor and Xamarin.Forms user!

@jfversluis jfversluis closed this Apr 25, 2024
v5.0.1 automation moved this from Reviewer approved to Done Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

[Bug] GroupHeaderTemplate leaks Memory