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
[ios/catalyst] fix memory leaks in *Cell
#22067
Conversation
Needs a rebase to get the updated snapshot for Issue18242Test and fix the build. |
86e9d49
to
08d9594
Compare
@jsuarezruiz do you know what's wrong with the one test lane? I wonder if it is caused by the changes here. |
/rebase |
a9ff7f9
to
0e57c49
Compare
Context: https://github.com/AdamEssenmacher/MemoryToolkit.Maui/tree/main/samples Retesting this sample with the `ListView` leak fixed, I noticed the sample reports 0 leaks! Unfortunately, it still displayed memory growing over time... Taking a `.gcdump` snapshot, I noticed a cycle: * `ViewCellRenderer.ViewTableCell` -> `ViewCell _viewCell` -> `ViewCellRenderer` -> `ViewTableCell` I was able to write a test that reproduces the leak, and I extended it for every type of `*Cell` like `TextCell`, `ImageCell`, `SwitchCell`, etc. The fixes are: * `ViewTableCell` now uses a `WeakReference<ViewCell> _viewCell` * `CellTableViewCell` now uses a `WeakReference<Cell> _cell` * `CellTableViewCell` now uses a `WeakEventManager` for `PropertyChanged`, as the `*Renderer` subscribes to this event. Note that I changed `PropertyChanged` to an event, which is an API change. (I can revisit this if needed)
One test was crashing (even on rerun), so maybe this will fix
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.
I'm going to try to rework this without needing the InternalPropertyChanged
@@ -135,44 +133,9 @@ protected void UpdateBackground(UITableViewCell tableViewCell, Cell cell) | |||
SetBackgroundColor(tableViewCell, cell, uiBgColor); | |||
} | |||
|
|||
[Obsolete("The ForceUpdateSizeRequested event is now managed by the command mapper, so it's not necessary to invoke this event manually.")] |
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.
This method was just a really complicated way of wiring up to _onForceUpdateSizeRequested
and making sure to unwire from it.
We can just achieve this same thing through the commandmapper
now
else | ||
{ | ||
tvc.PropertyChanged -= HandlePropertyChanged; | ||
CellPropertyChanged -= HandlePropertyChanged; |
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.
Because this event now is just local to this class this isn't going to cause a circular reference. I opted to keep this event path because this maintains the timing of when HandlePropertyChanged
will start and stop firing
I reworked most of this PR so my review isn't relevant anymore
Fixes: #20195
Context: https://github.com/AdamEssenmacher/MemoryToolkit.Maui/tree/main/samples
Retesting this sample with the
ListView
leak fixed, I noticed the sample reports 0 leaks! Unfortunately, it still displayed memory growing over time...Taking a
.gcdump
snapshot, I noticed a cycle:ViewCellRenderer.ViewTableCell
->ViewCell _viewCell
->ViewCellRenderer
->ViewTableCell
I was able to write a test that reproduces the leak, and I extended it for every type of
*Cell
likeTextCell
,ImageCell
,SwitchCell
, etc.The fixes are:
ViewTableCell
now uses aWeakReference<ViewCell> _viewCell
CellTableViewCell
now uses aWeakReference<Cell> _cell
CellTableViewCell
now uses aWeakEventManager
forPropertyChanged
, as the*Renderer
subscribes to this event.Note that I changed
PropertyChanged
to an event, which is an API change. (I can revisit this if needed)