Use ICollection<T> check instead of IList<T> in TryCopyTo

[prozolic]

#88181

This PR change the type check from IList to ICollection. With this change, collections that implement ICollection will have ICollection.CopyTo invoked.

• Change the type check from IList<T> to ICollection<T>
• If the type does not match List<T>, T[], or ImmutableArray<T>, fall back to ICollection<T>.CopyTo
• Add conditional compilation( #if NET) to ensure the sequence is not an array before falling back to ICollection<T>.CopyTo to avoid issues with array covariance in .NET Framework

Benchmark

#88181 (comment)

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

[Copilot]

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> to ICollection<T>, broadening the fast-copy path to any ICollection<T> implementation.
• After the well-known-type fast paths (List<T>, exact T[], ImmutableArray<T>), falls through to a generic ICollection<T>.CopyTo call for all other implementors.
• Adds a #if !NET guard to skip arrays that are not exactly T[] (covariant arrays) when running on .NET Framework, where ICollection<T>.CopyTo on such arrays can throw ArrayTypeMismatchException.

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

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.

[stephentoub]

https://github.com/dotnet/runtime/pull/125304/changes#diff-79ebb1b84d82a6117697d669c596bdcb999803d9ecf5a5339d6431d1ac0c5f7eR93-R96 suggests the lack of this is on purpose

[prozolic]

Thanks for the pointer. You're right that the original IList restriction was intentional.
The implementation approach follows what was discussed and considered in #88181 (#88093 (comment)).
@adamsitnik also confirmed in #88181 that the benchmark results look good.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

[tannergooding]

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.

[prozolic]

Thanks for the suggestion. I think that simplifying to the array special handling and ICollection<T>.CopyTo is a reasonable approach for .NET, where DPGO and guarded devirtualization can optimize the virtual dispatch.
How about something like this?

        internal static bool TryCopyTo<T>(this IEnumerable<T> sequence, T[] array, int arrayIndex)
        {
            if (sequence is ICollection<T> collection)
            {
#if !NET
                if (collection.GetType() != typeof(T[]) && collection is Array)
                {
                    return false;
                }
#endif
                collection.CopyTo(array, arrayIndex);
                return true;
            }

            return false;
        }

My concern is whether this could regress performance on .NET Framework, where those optimizations are not available. If that's an issue, a #if NET split to preserve the explicit type checks on .NET Framework might be worth considering.
I'd like to hear others' thoughts on both the simplified approach itself and whether a #if NET split would be needed.

[jkotas]

Microbenchmark numbers are the best way to decide questions like this.

[prozolic]

The benchmark results are as follows. On net472, the simplified version regresses only for List<T>.
On net11, I think that there is no regression for any collection type.

• net472

Method	input	Mean	Error	StdDev	Median	Min	Max	Allocated
PR	Array	48.788 ns	0.7753 ns	0.7252 ns	48.442 ns	48.154 ns	50.912 ns	-
PR_Simplified	Array	49.233 ns	1.9642 ns	2.2620 ns	48.398 ns	48.169 ns	57.831 ns	-
PR	DictValues	91.992 ns	3.7286 ns	4.2939 ns	90.405 ns	89.813 ns	106.749 ns	-
PR_Simplified	DictValues	91.521 ns	2.7121 ns	3.1232 ns	90.104 ns	89.858 ns	102.573 ns	-
PR	HashSet	106.052 ns	0.6292 ns	0.5886 ns	105.884 ns	105.413 ns	107.558 ns	-
PR_Simplified	HashSet	105.879 ns	1.5209 ns	1.4227 ns	105.522 ns	105.163 ns	110.951 ns	-
PR	ICollection	24.860 ns	1.1025 ns	1.2696 ns	24.386 ns	24.270 ns	29.677 ns	-
PR_Simplified	ICollection	24.277 ns	0.1839 ns	0.1720 ns	24.216 ns	23.970 ns	24.605 ns	-
PR	IEnumerable	5.787 ns	0.6049 ns	0.6966 ns	5.521 ns	5.476 ns	8.122 ns	-
PR_Simplified	IEnumerable	4.865 ns	0.0238 ns	0.0223 ns	4.858 ns	4.830 ns	4.912 ns	-
PR	IList	25.111 ns	0.6291 ns	0.7245 ns	24.879 ns	24.662 ns	27.545 ns	-
PR_Simplified	IList	24.636 ns	0.4881 ns	0.4566 ns	24.504 ns	24.384 ns	26.241 ns	-
PR	ImmutableArray	20.858 ns	2.1911 ns	2.5233 ns	19.807 ns	19.734 ns	29.216 ns	-
PR_Simplified	ImmutableArray	22.977 ns	0.1145 ns	0.1071 ns	22.964 ns	22.781 ns	23.206 ns	-
PR	List	13.092 ns	1.1985 ns	1.3803 ns	12.505 ns	11.902 ns	16.128 ns	-
PR_Simplified	List	19.011 ns	0.0879 ns	0.0823 ns	18.986 ns	18.901 ns	19.233 ns	-

• net11

Method	input	Mean	Error	StdDev	Median	Min	Max	Allocated
PR	Array	6.1776 ns	0.1712 ns	0.1971 ns	6.1610 ns	5.8437 ns	6.4629 ns	-
PR_Simplified	Array	6.6889 ns	0.6831 ns	0.7867 ns	6.3118 ns	6.2194 ns	9.2689 ns	-
PR	DictValues	56.8282 ns	6.2581 ns	7.2069 ns	53.3827 ns	51.5016 ns	74.8816 ns	-
PR_Simplified	DictValues	51.9682 ns	0.1728 ns	0.1616 ns	51.9340 ns	51.7708 ns	52.4342 ns	-
PR	HashSet	86.3227 ns	0.8258 ns	0.7725 ns	86.0793 ns	85.6178 ns	88.7023 ns	-
PR_Simplified	HashSet	79.2293 ns	0.8654 ns	0.8095 ns	79.0607 ns	78.5970 ns	81.9988 ns	-
PR	ICollection	8.1243 ns	0.7820 ns	0.9005 ns	7.8809 ns	7.5083 ns	11.5654 ns	-
PR_Simplified	ICollection	6.2598 ns	0.1683 ns	0.1938 ns	6.2204 ns	5.9885 ns	6.6185 ns	-
PR	IEnumerable	1.0592 ns	0.0048 ns	0.0045 ns	1.0592 ns	1.0519 ns	1.0665 ns	-
PR_Simplified	IEnumerable	0.8723 ns	0.0056 ns	0.0052 ns	0.8708 ns	0.8667 ns	0.8830 ns	-
PR	IList	8.6967 ns	0.9371 ns	1.0792 ns	8.2324 ns	7.9107 ns	12.1146 ns	-
PR_Simplified	IList	6.3230 ns	0.1689 ns	0.1946 ns	6.3095 ns	5.9923 ns	6.7738 ns	-
PR	ImmutableArray	6.8573 ns	0.7276 ns	0.8379 ns	6.5145 ns	6.3095 ns	9.6131 ns	-
PR_Simplified	ImmutableArray	6.2418 ns	0.6434 ns	0.7410 ns	6.0347 ns	5.6379 ns	8.4593 ns	-
PR	List	6.9636 ns	1.0816 ns	1.2456 ns	6.2800 ns	5.7477 ns	9.2334 ns	-
PR_Simplified	List	6.7189 ns	1.2294 ns	1.4158 ns	5.9877 ns	5.3658 ns	9.1378 ns	-

Benchmark source

    [MemoryDiagnoser]
    [BenchmarkCategory(Categories.Libraries, Categories.Collections)]
    public class Perf_TryCopyTo
    {
        private int[] _dest;

        public IEnumerable<object> Arguments()
        {
            yield return TestData.IEnumerable;
            yield return TestData.ICollection;
            yield return TestData.IList;
            yield return TestData.Array;
            yield return TestData.List;
            yield return TestData.ImmutableArray;
            yield return TestData.DictValues;
            yield return TestData.HashSet;
        }

        [GlobalSetup]
        public void Setup()
        {
            _dest = new int[100];
        }

        [Benchmark] 
        [ArgumentsSource(nameof(Arguments))]
        [MemoryRandomization]
        public bool PR(TestData input) => TryCopyTo(input.Collection, _dest, 0);

        [Benchmark]
        [ArgumentsSource(nameof(Arguments))]
        [MemoryRandomization]
        public bool PR_Simplified(TestData input) => TryCopyTo_Simplified(input.Collection, _dest, 0);

        static bool TryCopyTo<T>(IEnumerable<T> sequence, T[] array, int arrayIndex)
        {
            if (sequence is ICollection<T> collection)
            {
                if (collection is List<T> list)
                {
                    list.CopyTo(array, arrayIndex);
                    return true;
                }

                if (collection.GetType() == typeof(T[]))
                {
                    var sourceArray = (T[])collection;
                    Array.Copy(sourceArray, 0, array, arrayIndex, sourceArray.Length);
                    return true;
                }

                if (collection is ImmutableArray<T> immutable)
                {
                    immutable.CopyTo(array, arrayIndex);
                    return true;
                }

                if (collection is Array)
                {
                    return false;
                }

                collection.CopyTo(array, arrayIndex);
                return true;
            }
            return false;
        }

        static bool TryCopyTo_Simplified<T>(IEnumerable<T> sequence, T[] array, int arrayIndex)
        {
            if (sequence is ICollection<T> collection)
            {
#if !NET
                if (collection.GetType() != typeof(T[]) && collection is Array)
                {
                    return false;
                }
#endif
                collection.CopyTo(array, arrayIndex);
                return true;
            }
            return false;
        }
    }

    public class TestData
    {
        internal const int Size = 100;

        private static readonly int[] array = Enumerable.Range(0, Size).ToArray();
        internal static readonly TestData Array = new(array);
        internal static readonly TestData List = new(new List<int>(array));
        internal static readonly TestData ImmutableArray = new(Collections.Immutable.ImmutableArray.Create(array));
        internal static readonly TestData DictValues = new(array.ToDictionary(x => x, x => x).Values);
        internal static readonly TestData HashSet = new(new HashSet<int>(array));
        internal static readonly TestData IEnumerable = new(ToEnumerable(Size));
        internal static readonly TestData IList = new(new IListWrapper<int>(array));
        internal static readonly TestData ICollection = new(new ICollectionWrapper<int>(array));

        private TestData(IEnumerable<int> collection) => Collection = collection;

        internal IEnumerable<int> Collection { get; }

        private static IEnumerable<int> ToEnumerable(int size)
        {
            for (int i = 0; i < size; i++)
            {
                yield return i;
            }
        }

        public override string ToString()
        {
            switch (Collection)
            {
                case int[] _:
                    return "Array";
                case List<int> _:
                    return "List";
                case ImmutableArray<int> _:
                    return "ImmutableArray";
                case Dictionary<int, int>.ValueCollection _:
                    return "DictValues";
                case HashSet<int> _:
                    return "HashSet";
                case IList<int> _:
                    return "IList";
                case ICollection<int> _:
                    return "ICollection";
                default:
                    return "IEnumerable";
            }
        }

        private class ICollectionWrapper<T> : ICollection<T>
        {
            private readonly T[] _array;
            public ICollectionWrapper(T[] array) => _array = array;

            public IEnumerator<T> GetEnumerator() => ((IEnumerable<T>)_array).GetEnumerator();
            Collections.IEnumerator Collections.IEnumerable.GetEnumerator() => ((IEnumerable<T>)_array).GetEnumerator();

            public int Count => _array.Length;
            public bool IsReadOnly => true;
            public bool Contains(T item) => System.Array.IndexOf(_array, item) >= 0;
            public void CopyTo(T[] array, int arrayIndex) => _array.CopyTo(array, arrayIndex);

            public void Add(T item) => throw new NotImplementedException();
            public void Clear() => throw new NotImplementedException();
            public bool Remove(T item) => throw new NotImplementedException();
        }

        private class IListWrapper<T> : IList<T>
        {
            private readonly T[] _array;
            public IListWrapper(T[] array) => _array = array;

            public IEnumerator<T> GetEnumerator() => ((IEnumerable<T>)_array).GetEnumerator();
            Collections.IEnumerator Collections.IEnumerable.GetEnumerator() => ((IEnumerable<T>)_array).GetEnumerator();

            public int Count => _array.Length;
            public bool IsReadOnly => true;
            public T this[int index]
            {
                get { return _array[index]; }
                set { throw new NotImplementedException(); }
            }
            public bool Contains(T item) => System.Array.IndexOf(_array, item) >= 0;
            public void CopyTo(T[] array, int arrayIndex) => _array.CopyTo(array, arrayIndex);
            public int IndexOf(T item) => System.Array.IndexOf(_array, item);

            public void Add(T item) => throw new NotImplementedException();
            public void Clear() => throw new NotImplementedException();
            public bool Remove(T item) => throw new NotImplementedException();
            public void Insert(int index, T item) => throw new NotImplementedException();
            public void RemoveAt(int index) => throw new NotImplementedException();
        }
    }