Mvvmcross: MemoryIssue with MvxRecyclerview and ObservableCollection

[Noires]

Hello,
iam currently having an issue that i have crashes with MvxNotifyPropertyChanged in Combination with SIGSEGV as described here: http://stackoverflow.com/questions/37585394/mvxnotifypropertychanged-application-crash-on-setproperty
At the moment i assume it is connected to the MvxRecyclerview and the ObservableCollection, but i would be happy if you would just proof me wrong and help me fix that issue.

So basically if i have a MvxRecyclerview connected to a ObservableCollection and just keep adding elements and clearing it, it creates more and more data objects which doesnt get freed.
Here is the repository where i tested it with, after seperating it from my project:
Github: https://github.com/Noires/MvxRecyclerViewLeakTest

Steps to reproduce

Repeatedly click the Button and watch the Heap in the Android Device Monitor.

Expected behavior

Allocated and the DataObjects should stay close to constant.

Actual behavior

Allocated and the DataObjects increase slowly.

Configuration

Xamarin Version: 4.0.4.4

Version: 4.1.6/4.1.7(RecyclerView)

Platform: Android/Xamarin

[Noires]

Might be related to #1345 ?
If it is, when would it be possible to put a new version on nuget?
Or rather would it be possible to even update it today?

I can understand if that is not possible, but our application will be presented soon and today is the last day we can make changes to it.

[Cheesebaron]

Sorry, it won't happen today. You always have the possibility to build from source.

[Noires]

Edith: Not sure if it didnt just free the objects when expected it, atleast no crash in my main application yet. Atm testing more.

[Cheesebaron]

This is hopefully fixed by https://github.com/MvvmCross/MvvmCross-AndroidSupport/commit/3b5c82b81e401904707ccd349c90139c9561895c

[ghost]

@Cheesebaron it's not fixed.
I just tried 4.2.0 and I noticed using a MvxRecyclerView brings the app to a halt, taking several seconds to do perform. Switched to MvxListView and issue goes away. This issue is not present in 4.1.0.

[ghost]

It causes a bunch of garbage collection and adds several seconds to execution time, very slow.

[Cheesebaron]

Will have a look tonight to see if we can get a fix for this.

[kjeremy]

I'm not seeing a problem with your sample app in either MvvmCross version on my physical device. I get a GC when hitting the "Memory Leak" button but scrolling is smooth and there aren't any hiccups. Are you seeing this on a physical device or emulator only?

[ghost]

I'm seeing it on a physical device and emulator. I haven't tried his sample
app. It will take me a bit to prep a sample as my code is Corp owned.

I suggest having a form bound to the selected item in the list, can be just
text views. I'm using a "TrulyObsservableCollection" in my case.

As I said before 4.1.0 does not have this issue but 4.1.5 did, so I'll try
to spot the code change that caused it.

[kjeremy]

What is a TrulyObservableCollection? In any case a sample repro app would be great.

[ghost]

    public sealed class TrulyObservableCollection<T>: ObservableCollection<T> where T: INotifyPropertyChanged
    {
        public TrulyObservableCollection()
        {
            CollectionChanged += FullObservableCollectionCollectionChanged;
        }

        public TrulyObservableCollection(IEnumerable<T> pItems): this()
        {
            foreach (var item in pItems)
            {
                Add(item);
            }
        }

        private void FullObservableCollectionCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
        {
            if (e.NewItems != null)
            {
                foreach (var item in e.NewItems)
                {
                    ((INotifyPropertyChanged)item).PropertyChanged += ItemPropertyChanged;
                }
            }
            if (e.OldItems == null)
            {
                return;
            }
            {
                foreach (var item in e.OldItems)
                {
                    ((INotifyPropertyChanged)item).PropertyChanged -= ItemPropertyChanged;
                }
            }
        }

        private void ItemPropertyChanged(object sender, PropertyChangedEventArgs e)
        {
            var args = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, sender, sender, IndexOf((T)sender));
            OnCollectionChanged(args);
        }
    }

[Cheesebaron]

That is a leaky collection... Why are you doing this?

[ghost]

It wasn't me, my teammate is convinced its needed so properties underneath a class in the collection that get changed will raise the event and update the binding.

Even though the classes and properties themselves implement property changed.

[Cheesebaron]

It is not necessary at all... Your collection is leaking the CollectionChanged event.

[kjeremy]

@robertbaker : As long as the item in the collection implements INotifyPropertyChanged (and in most cases the change occurs on the UI thread) it should work. If it doesn't then that's a bug. Be gentle with your fragile teammate but be firm.

[kjeremy]

As for a memory leak in the sample program provided if I spam the button the working set stays the same while allocated memory continuously increases so there may be a leak (live objects do seem to increase). I thought the issue was with scrolling until I read the stackoverflow post.

[kjeremy]

Ok I'm pretty sure there's a leak where the MvxRecyclerViewHolder stays around. This might have to do with assigning the click event handlers: https://github.com/MvvmCross/MvvmCross-AndroidSupport/blob/master/MvvmCross.Droid.Support.V7.RecyclerView/MvxRecyclerAdapter.cs#L135-L139

[Cheesebaron]

So you suspect it being the two commands keeping the reference alive? You might be right, they are never let go.

[kjeremy]

I'm thinking maybe they should be assigned/unassigned in onViewAttachedToWindow/onViewDetachedFromWindow. I'll try to do that locally and see.

[Cheesebaron]

Maybe we could do something with weakreferences, but assigning and unassigning in those to methods should protentially work.

[kjeremy]

In the case of the sample program these events are always null so that isn't the issue here. That said this is probably a good idea.

[kjeremy]

I'm torn as to whether this is a bug or not... it may not be or maybe there's room for optimization:

It looks like a number of MvxRecyclerViewHolder's are created. These hold onto the view and the binding context. Changing the ItemSource by either reassignment or Clear() doesn't unassign the view or binding context already held in the MvxRecyclerViewHolder consequently the old ListItem, View etc are all alive. Some of these objects get collected after a while but there are typically quite a few of the old ones left around. It also appears that holding onto a List[i] keeps the entire collection alive so as long as one of the list items is alive the ObservableCollection's underlying array is alive which might account for some of the memory issues. If we had a way of suspending the bindings while an item is off screen and hooking it back up then maybe we could relieve some of the memory pressure (there's a lot that comes along with a MvxAndroidBindingContext)

Unfortunately I don't have enough confidence in the Xamarin profiler to say whether I'm seeing the complete picture or not. It tends to lock the app on me a lot and I don't trust its measurements at all.

[kjeremy]

Just do be clear we do unset the DataContext at the right place in the view holder and then set it back in OnBindView and when we reattach the VH. Maybe we need to set the _cachedDataContext to null if we set the DataContext to something other then null in the VH. That could reduce the references to stale data.

[kjeremy]

The pull from MvvmCross/MvvmCross-AndroidSupport#268 should fix most if not all of this. @Noires and @robertbaker please test with this pull.

There is one slightly weird leak that I'm seeing that may not be that big of a deal but it's... odd. You can clear out the ItemsSource or even reassign the variable but you will still leak the original ItemsSource's underlying array in the case that ItemsSource is an ObservableCollection.

For example in the sample program we leak an empty array (32 bytes) that backs ObservableCollection<ListItem> TestList even if you set TestList to a new instance. Not a big deal in this case since the array is empty.

It looks to be held by a MonoProperty.Getter that is ultimately held by some other stuff:

[Noires]

I will try to do it soonish, just hard to do at workplace. Hopefully iam able to check it out tomorrow.