-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fixes various memory leaks in listview with group headers (#6761) #12535
Conversation
Hi and thank you so much for these fixes!! What release will this fix be in? |
@Tommigun1980 the PR is against 5.0.0. I am not sure why this is waiting so long to get merged though. |
Waiting for a reviewer... |
Hi! Any chance of getting this memory leak fix in at some point? Thanks! |
/azp run |
Commenter does not have sufficient privileges for PR 12535 in repo xamarin/Xamarin.Forms |
@jsuarezruiz any chance you can trigger the pipeline for this PR also? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@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
@jsuarezruiz can you kick the build again |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@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. |
@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! |
@StephaneDelcroix this is waiting your review? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this 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
@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. |
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! |
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
(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