Performance: Make constructor List<T>(IEnumerable<T> collection) know about IReadOnlyCollection

[dnickless]

In the following List<T> constructor:

https://github.com/dotnet/corefx/blob/e043326e204a9566ee26403b522cf95cda2d54ee/src/Common/src/CoreLib/System/Collections/Generic/List.cs#L67

there is some special logic built-in already to support the efficient initialization of a List<T> based on an existing ICollection<T>. With the advent of IReadOnlyCollection<T> this constructor logic could be extended to also support this type.

IReadOnlyCollection<T> does not support a CopyTo() method at this stage but at least the cost of array resizes could be alleviated here at the expense of another branch/type check.

[Wraith2]

This will cause a performance degradation in existing code, the majority will pay the cost for something they are unlikely to use and have no opportunity to opt out of it. Casting is relatively slow and generic casts are slower still and can cause jit-ing of the IReadonlyCollection type if it isn't used already.

Adding code to core parts of the clr like Dictionary and List has a multiplicative cost in size because of native copies of common types are precompiled for performance. While that doesn't mean common methods can't increase in size it makes the bar for change approval on high-use types is higher than you'd expect and that evidence of improvement must be considerable.

In this case you could provide an Extension method to create a list from a readonly collection that presizes using the existing api couldn't you?

[stephentoub]

dotnet/corefx#28472
https://github.com/dotnet/corefx/issues/26579

[dnickless]

Well, that is not exactly unexpected...

I just keep coming across this on a fairly regular basis... It popped up in a trace again today - in fact, it wasn't even that List<T> constructor call directly, it was rather LINQ's .ToList<T> in with a parameter of type Dictionary<K, V>.KeyCollection (which implements IReadOnlyCollection<T> only). So initially, I thought of suggesting this change for LINQ's ToList<T> but this already does some magic using the ListProvider interface... It feels a bit like we have a lot of not entirely consistent approaches here.

This IReadOnlyCollection<T> is just one of those less obvious buggers that give you unfortunate surprises because one collection behaves in one way and another one in a different way.

And while I do think that it's unfortunate, that e.g. ICollection<T> does not inherit from IReadOnlyCollection<T> I also think we've covered this elsewhere and there are good reasons so I'll close this one.