Use ICollection<T> check instead of IList<T> in TryCopyTo#125304
Use ICollection<T> check instead of IList<T> in TryCopyTo#125304prozolic wants to merge 6 commits intodotnet:mainfrom
Conversation
This PR change the type check from IList<T> to ICollection<T>. With this change, collections that implement ICollection<T> will have ICollection<T>.CopyTo invoked.
|
Tagging subscribers to this area: @dotnet/area-system-collections |
There was a problem hiding this comment.
Pull request overview
This PR changes ImmutableExtensions.TryCopyTo to use ICollection<T> as the type gate instead of IList<T>. This allows collections that implement ICollection<T> but not IList<T> (such as Dictionary<K,V>.Keys and Dictionary<K,V>.Values) to benefit from the fast CopyTo path rather than falling back to per-element enumeration. The performance motivation is that LINQ's own ToArray already takes this ICollection<T> path, and the previous restriction was making immutable collection operations unnecessarily slower.
Changes:
- Changes the outer type check from
IList<T>toICollection<T>, broadening the fast-copy path to anyICollection<T>implementation. - After the well-known-type fast paths (
List<T>, exactT[],ImmutableArray<T>), falls through to a genericICollection<T>.CopyTocall for all other implementors. - Adds a
#if !NETguard to skip arrays that are not exactlyT[](covariant arrays) when running on .NET Framework, whereICollection<T>.CopyToon such arrays can throwArrayTypeMismatchException.
You can also share your feedback on Copilot code review. Take the survey.
...System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs
Outdated
Show resolved
Hide resolved
...System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs
Outdated
Show resolved
Hide resolved
...System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs
Outdated
Show resolved
Hide resolved
…ons/Immutable/ImmutableExtensions.Minimal.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
...System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs
Show resolved
Hide resolved
|
https://github.com/dotnet/runtime/pull/125304/changes#diff-79ebb1b84d82a6117697d669c596bdcb999803d9ecf5a5339d6431d1ac0c5f7eR93-R96 suggests the lack of this is on purpose |
|
Thanks for the pointer. You're right that the original IList restriction was intentional. |
...System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs
Outdated
Show resolved
Hide resolved
...System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs
Show resolved
Hide resolved
...System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs
Outdated
Show resolved
Hide resolved
...System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs
Outdated
Show resolved
Hide resolved
| if (sequence is ICollection<T> collection) | ||
| { | ||
| if (sequence is List<T> list) | ||
| if (collection is List<T> list) |
There was a problem hiding this comment.
Are all these secondary type checks (where it has to enumerate through the types/interfaces to see if it is that type) better than just doing collection.CopyTo(...)?
I'd expect the virtual dispatch overhead, especially with DPGO/guarded-devirtualization to be cheaper and that we could get this down to basically just the type check for ICollection and one more for the array special handling for the common path. Rather than needing up to 4-5.
#88181
This PR change the type check from IList to ICollection. With this change, collections that implement ICollection will have ICollection.CopyTo invoked.
IList<T>toICollection<T>List<T>,T[], orImmutableArray<T>, fall back toICollection<T>.CopyTo#if NET) to ensure the sequence is not an array before falling back toICollection<T>.CopyToto avoid issues with array covariance in .NET FrameworkBenchmark
#88181 (comment)