jtmueller / Collections.Pooled

Fast, low-allocation ports of List, Dictionary, HashSet, Stack, and Queue using ArrayPool and Span.
MIT License
547 stars 48 forks source link

Support for ObservableCollection #32

Open Symbai opened 5 years ago

Symbai commented 5 years ago

Would be amazing if you could add support for ObservableCollection as it's the only collection type that fully supports binding. If you add support I would recommend or suggest you also add a AddRange method as the original collection hasn't but for performance reasons it's a must-have. There are many modified versions that added such as a method but none which is optimized with ArrayPool etc.

jtmueller commented 5 years ago

I've got some preliminary code going in a branch.

For AddRange, what would you expect to happen to the CollectionChanged events that would normally fire for each item added?

Symbai commented 5 years ago

Thanks for getting back to this. This is my custom AddRange implementation for the ObservableCollection which I've been using for a long time now. There are a few cases where you would want it to fire per item but for me this is too slow.

public virtual void AddRange(IEnumerable<T> items)
        {
            Execute.OnUIThreadSync(() =>
            {
                OnCollectionChanging(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));

                var previousNotificationSetting = isNotifying;
                isNotifying = false;
                var index = Count;
                foreach (var item in items)
                {
                    base.InsertItem(index, item);
                    index++;
                }
                isNotifying = previousNotificationSetting;
                OnPropertyChanged(new PropertyChangedEventArgs("Count"));
                OnPropertyChanged(new PropertyChangedEventArgs("Item[]"));
                OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
            });
        }
jtmueller commented 5 years ago

Unless things have changed since 2009, it seems like the main motivation for having AddRange is to avoid firing an event for each item added. However, at least at the time of writing, "ListCollectionView doesn’t implement support for non-single item CollectionChanged events."

https://blogs.msdn.microsoft.com/nathannesbit/2009/04/20/addrange-and-observablecollection/

The features/observable-collection branch currently implements the "single event with everything that was added" approach, but it may not be compatible with some commonly-used WPF controls. Hopefully those controls have been updated since 2009 and it just works. If not, maybe I could implement a workaround like this.

@Symbai any way you could test it out, see if the extra workaround is needed or not? I should mention that branch requires you to have the latest .NET Core 3 preview SDK to build it.

Symbai commented 5 years ago

I've been using my custom AddRange for a lot of WPF controls now without having any issues at all. A month ago it was officially added in .NET Core 3 repo, but later it got revert https://github.com/dotnet/corefx/pull/38115

Anyway, in my case the extra workaround is not required. Tested it and it works fine and is faster than the ObservableCollection for many items, nice work!

Symbai commented 5 years ago

Fyi I'm closing this, hope that's okay

jtmueller commented 5 years ago

@Symbai That's great that it's working for you! However, I'm going to re-open this for now, as I haven't ported over any of the unit tests or benchmarks for Collection/ObservableCollection, and thus I'm not ready to merge the branch in question yet and include it in the NuGet package.

I'll close the issue when I merge the observable-collection branch, after I've got unit tests and benchmarks running, and I'm satisfied with the benchmark results. Thanks for checking it out.