Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IReadOnlyCollection<T> does not implement ICollection<T> #33245

Closed
wants to merge 1 commit into from
Closed

IReadOnlyCollection<T> does not implement ICollection<T> #33245

wants to merge 1 commit into from

Conversation

Pentadome
Copy link

No description provided.

@GrabYourPitchforks
Copy link
Member

@stephentoub Does the order of these checks matter?

@stephentoub
Copy link
Member

stephentoub commented Mar 5, 2020

Does the order of these checks matter?

Just having the check itself matters. We've rejected this same change multiple times in the past, as it adds overhead to the common non-collection path and it's relatively rare to have IReadOnlyCollection types that aren't also ICollection. It's possible things have changed perf-wise with @VSadov's recent casting work, but justifying this would need new numbers.

@VSadov
Copy link
Member

VSadov commented Mar 5, 2020

Cost of checking for ICollection<T> may vary depending where it is in the implementation list of a given type. It is not exactly linear due to cache alignment effects and for short implementation lists, which is common, will not vary much.
Checking for IReadOnlyCollection<T> on common types like List<T> is comparable to that.
(see rough numbers in: #1068 (comment))

So - yes, it is no longer a special expensive cast, but not free either. Basically it is just another type check that needs to be amortized by potential benefits.

@@ -20,6 +20,10 @@ public static bool Any<TSource>(this IEnumerable<TSource> source)
{
return collectionoft.Count != 0;
}

if (source is IReadOnlyCollection<TSource> readOnlyCollection)
return readOnlyCollection.Count != 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use curly braces

@stephentoub stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 9, 2020
@stephentoub
Copy link
Member

stephentoub commented Mar 11, 2020

There are ~20 places across the System.Linq implementation today where we check for ICollection<T>, a handful of which do so just to access Count. If we want to start factoring in IReadOnlyCollection<T> instead of or in addition to ICollection<T>, I would like to see us do so holistically, meaning a) doing so consistently across the library and b) validating any performance impact to doing so (even with @VSadov's perf improvements around casting, based on measurements I took this morning, there's still a measureable impact, in particular for small-ish enumerables). That also means considering that a significant percentage of the collection types in use that implement IReadOnlyCollection<T> also implement ICollection<T>, so the positive benefits of such checks in many situations will be limited. Given all that, I'm going to close this PR, which just adds a check in one place and without associated justification. Regardless, thank you @wrkettlitz for your interest in contributing and for the PR!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Linq NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants