Runtime: Respect IReadOnlyList<T> in the BCL

[weitzhandler]

The following condition appears in many variations plenty of times throughout the BCL, especially in LINQ, (From ElementAt):

c# if (source is IList<TSource> list) { return list[index]; }

But there is also IReadOnlyList<T> which provides the same thing and is not considered because it doesn't overlap with IList<T>, and the fact that random access is provided by it is neglected in the BCL.
Why is that?

Similar to ICollection<T> and IReadOnlyCollection<T>, see dotnet/runtime#23337.

[ikopylov]

The main reason for that is the performance overhead: every additional type cast adds its own cost and the cost of cast to variant generic interface is much higher.

Corresponding issue https://github.com/dotnet/coreclr/issues/603

[weitzhandler]

But going O(n) on a random access collection?

[ikopylov]

Due to historical reasons all collections from BCL that implement IReadOnlyList<T> also implement IList<T> (even immutable ones). For the third-party libraries this is also true in most cases. As a result, in majority number of sitations it is enough to check only for IList<T> implementation.

LINQ is often used on a hot path, so an additional type check may result in regression for most use-cases. ElementAt is somewhat special and perhaps can also check for IReadOnlyList<T>. But that change should be tested by benchmarks and in the real-world scenarious.

[weitzhandler]

So what's the point I implementing IReadOnlyList<T> as a gateway to random access if not always respected by the BCL?

[rvhuang]

For the third-party libraries this is also true in most cases.

@ikopylov That always sounds a bit weird to me because it suggests that IList<T> and IReadOnlyList<T> have underlying connection and have been implicitly coupled but they actually shouldn't (same situation for ICollection<T> and IReadOnlyCollection<T>).

If we can't add read-only interface check to existing Enumerable extensions, the only solution that I can come up with so far is to provide a new set of extension methods that are specially optimized for index-supported series. Just like we have AsParallel() which enables PLINQ and AsQueryable() which enables expressions, we can also have something like AsIndexable() to enable the wrapper.

I know my suggestion sounds stupid. Feel free to laugh at me. 😞

[svick]

@rvhuang I don't think your suggestion is stupid. But I do think it has roughly zero chance of being included in corefx. That's because it might offer some performance improvements in theory, but it won't do that in practice. And it doesn't make sense to add an API whose only purpose is to improve performance, if it won't actually improve performance.

And if you have a library of collections where AsIndexable() would help, you could include it in that library.

[rvhuang]

That's because it might offer some performance improvements in theory, but it won't do that in practice.

@svick In fact performance is not the thing I wanted to point out. The actual one is here as I mentioned.

it suggests that IList<T> and IReadOnlyList<T> have underlying connection and have been implicitly coupled but they actually shouldn't.

This is one thing that I would consider API design flaw as you can't implement IReadOnlyList<T> only or you will get random access.

[weitzhandler]

@rvhuang
This is one thing that I would consider API design flaw as you can't implement IReadOnlyList only or you will get random access.

Exactly! Same thing bother me with ICollection. There supposed to be a common Count property.

But performance also counts. Accessing a list in linear fashion when an index/count is available is waste of resources, especially with bigger lists (See ICountable dotnet/runtime#23337).

[Soon5]

This is one thing that I would consider API design flaw as you can't implement IReadOnlyList<T> only or you will get random access.

I second that. I was just implementing a new class with only Implementing IReadOnlyList. If I also have to implement IList so I can use it with LINQ properly, the whole Idea of having a ReadOnly Interface is gone.

[eiriktsarpalis]

This is related to #42254, I'm going to close this so that we can consolidate the conversation in one issue.