ObservableCollection.AddRange()

[robertmclaws]

• Introduce an AddRange function that waits until all new items are added to the collection before firing OnCollectionChanged.
• Moved a helper method out of the "Constructors" section for organizational clarity.

[dnfclas]

Hi @advancedrei, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[dnfclas]

@advancedrei, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[weshaggard]

@advancedrei lets please follow the process at http://aka.ms/apireview for adding new APIs.

[robertmclaws]

Oops, sorry @weshaggard. I'll open an issue. (UPDATE: It's open).

[stephentoub]

Thanks for the contribution. Per @weshaggard's comments and the issue you opened, I'm going to close this PR until the API is agreed upon. At that point this PR or a new one can be re-opened.

[justinvp]

A note for when opening a new PR based on finalized API discussed in #10752...

Right now Items is always going to be List<T>, so these operations can be optimized.

For example, for AddRange, instead of:

var newIndex = Items.Count;
foreach (var item in collection)
{
    base.InsertItem(newIndex, item);
    newIndex++;
}

It can be:

List<T> list = (List<T>)Items;
list.AddRange(collection);

Which will take advantage of fast-paths inside List<T>'s implementation.

AddRange, InsertRange, and RemoveRange, can all be implemented in this way.

[robertmclaws]

In reading up a bit about you, apparently you and @benaadams are "THE MEN" when it comes to perf optimizations in ,NET Core. So. looking at Collection.cs, I'm wondering if there are any other optimizations possible from working with the Items IList? Could just combine them all into the same PR. Thanks for the insight!

[justinvp]

Looking at this again, will subclasses expect base.InsertItem to be called for each item in the collection passed to AddRange? If so, we'd only be able to do the optimization if InsertItem hasn't been overridden. I'm not aware of an easy/fast way to determine that other than checking more broadly to see if the ObservableCollection<T> hasn't been subclassed (which we've done in a bunch of other places) e.g.:

if (GetType() != typeof(ObservableCollection<T>))
{
    // If we've been inherited into a subclass, we need to call InsertItem
    // for each item in the collection in case the subclass has overridden
    // the virtual InsertItem method.
    int newIndex = Items.Count;
    foreach (T item in collection)
    {
        base.InsertItem(newIndex, item);
        newIndex++;
    }
}
else
{
    // Otherwise, if we're not a subclass, there's no need to call InsertItem
    // for each item in the collection. Instead, we can call AddRange on the
    // underlying List<T>, which has optimized paths.
    List<T> list = (List<T>)Items;
    list.AddRange(collection);
}

I couldn't easily tell from the latest discussion in #10752 which APIs are now intended for Collection<T> vs. ObservableCollection<T>. Is AddRange going to be added to Collection<T>? If so, my example above will obviously have to be tweaked for typeof(Collection<T>) as well as a check added to see if Items is List<T> (as it might not be for Collection<T>, whereas with ObservableCollection<T>, it will currently always be List<T>), and then since ObservableCollection<T> is a subclass of Collection<T>, if we want it to have an optimized path, we'd have to override AddRange and provide an optimization for it along the lines of my example.

[robertmclaws]

It's going to be added to Collection<T>, and then ObservableCollection<T> will override to raise the events.

Regarding the typecheck, the internal Items IList for Collection.cs is a concrete List<T>, the new instance of which is created on Line 25. So I would think your code optimization would still apply.

As for the implementation and subclassing, I think that the best bet is going to be to continue to follow the existing pattern, which is to have a public method that calls a protected virtual method that subclasses can override. The "Range" passed in will always be IEnumerable<T>. The reason for adding these methods is to optimize group processing over individual processing, so the protected virtual method can use your optimized path, and subclassed collections can override the behavior and call ____Item if they need to.

[justinvp]

Regarding the typecheck, the internal Items IList for Collection.cs is a concrete List, the new instance of which is created on Line 25. So I would think your code optimization would still apply.

That's true for the default .ctor, but there's another .ctor (line 29) that takes any IList<T>.

[robertmclaws]

Damn. Good catch. According to this, usage of that constructor is not very high. Don't know if that matters. I know that constructor is used for ReadOnlyCollection, so probably needs better typechecking.

My initial commit is here: robertmclaws/coreclr@8416720 Feel free to leave comments now, or you can wait for the official PR.

[justinvp]

It'd be nice if we didn't have to allocate/copy-to a new List<T> if the passed-in collection already implements IList.

[weitzhandler]

Here's a prototype that I use. It's a subclass of ObservableCollection and is an enhancement of this. I find some of the original functionality redundant, but it updates the UI fast for me. I was facing a scenario where the collection updates ~5 times per second. Reusing items was essential.

[weitzhandler]

@robertmclaws
Are you still working on it or planning to? Otherwise I'd like to take over.