Fixes memory leak from grouped ListView with HasUnevenRows set #12447
Conversation
The Item created to measure was never removed, causing memory leak until the page is closed.
t-johnson
changed the title
|
Will this make it possible to set the GroupHeader to Height zero in iOS now? That would be amazing |
|
@Codelisk is there an issue logged about 0 height group headers? i don't think my change here would have any affect on the current behavior in this area, as I was just addressing memory leaks. |
|
@t-johnson this link details the issue. Basically at the moment it is not possible to make the group header invisible in ios it always takes space. |
|
@t-johnson okay, thank you very much! |
|
don't subscribe, I'm getting over 100 emails a day from this subscription |
...s.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
Show resolved
Hide resolved
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Something weird happened and the Build (also pass unit tests, ui tests etc) for this PR was not launched. I have launched the Build and will review it. |
|
Guys I tested this on build 5.0.0.6998 and it DOES NOT fix the memory leak issue we were seeing when using HasUnevenRows="True" |
|
@AlanYost How are you testing it? Given it has been several months since i last looked at this fix, i'll have to take some time to get my head back in this space... |
Hi @t-johnson - I have a test harness that I wrote that tracks the instance counts and where they are no longer considered active by the GC. I can provide this in a GITHUB repo if you are interested? |
|
@AlanYost how does your test harness compare to the test case added in this PR? If you can share it with me I can take a look. |
|
@t-johnson - I did not run the testcase in the PR - I downloaded the repo from the pipeline and upgraded our test harness. I'm just having macOS Big Sur upgrade issues at the moment that will require me to reboot etc. I will create a repo and send the link - hopefully before my machine decides it wants to die (what happened to Apple QA ... :-() |
|
@t-johnson - Please use the following repo to test. You may need to refer to your own XF nuget package as mine is sourced from a local version from the pipeline @nickrandolph - FYI: |
I've re-tested with the test case that I built here and still definitely still see an improvement. Its highly likely there are other memory leaks that are not triggered in my test. |
|
@t-johnson - Thanks for looking into it. I'm sure there are bugs too - Not sure how memory leaks are tested on XF - but it fails using our approach that we use in many other areas.... Did you try it? |
I've got your app running, using the latest stable release of Xamarin Forms. With that app, I'm seeing leaks for all the different views (ListView, ListViewGrouping, CollectionView) when the number of items is larger than what can be shown on the screen. This seems to be a very different case than what I've fixed in this PR. That said, I modified the test case i built for this PR to have more items than will fit on the screen, and I am still not seeing leaks in my test case... |
|
@t-johnson - You need to scroll thru the list and we also add/delete in the collection helps. Are you able to automate this in your tests? |
|
@AlanYost in your sample you are copying the items from the view model into an ObservableCollection in the View. its this collection that is used in the binding the ListView. If I change your sample so that the ListView binds directly to the ObservableCollection in the view model (along with appropriate changes to re-build that collection when the list is tapped) then the leak problem seems to be much reduced. regardless of number of items or scrolling, the sample only ever shows 1 instance of the View and ViewModel left behind. This one left over seems to be removed after navigating to a different view and then back, so it seems like the app is keeping the last-used View alive on purpose. |
|
@t-johnson - We have in the past tried different ways to bind to the collection and had the same results. Would it be possible for you to check in your code against my repo and I'll have a closer look? I really appreciate you looking into this and would love to know that it can be fixed an alternative way. |
rachelkang
moved this from Issue In progress
to PR Needs Review
in 5.0.0 SR 4 (Planning)
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Can you rebase please, to fix the cert failure? |
is it okay now? |
|
yes @t-johnson running iOS tests now. |
Description of Change
The native cell that is created in the GetEstimatedRowHeight was never removed, and was remaining as a child of the native list forever. This fix ensures that the cell is removed.
With this fix, we do not see the number of items increasing, and GC.GetTotalMemory is stable when resetting the collection the list is bound to.
Issues Resolved
API Changes
None
Platforms Affected
Behavioral/Visual Changes
None
Before/After Screenshots
Not applicable
Testing Procedure
Use the gallery to access test G6742. This test binds an ObservableCollection of ObservableCollections to a grouped listview which has HasUnevenRows = true.
Clicking the button in this test clears and rebuilds the collections. There are WeakReferences to the items in the collections, and we count the number of items kept after garbage collection. this number should remain stable, and not increase, as it was doing before the fix.
PR Checklist