Skip to content

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

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.

Sign up for GitHub

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

Open
prozolic wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dcdc850
Use ICollection<T> check instead of IList<T> in TryCopyTo
prozolic Mar 8, 2026
4754015
Add test data in ImmutableArrayTest.InsertRangeData
prozolic Mar 8, 2026
bc35c73
Update src/libraries/System.Collections.Immutable/src/System/Collecti…
prozolic Mar 8, 2026
4dff47d
Fix comment
prozolic Mar 8, 2026
a088eaa
Fix comment
prozolic Apr 10, 2026
4eb1db5
Store the result of ICollection<T> cast to avoid redoing
prozolic Apr 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions ...tem.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,10 @@ internal static bool TryCopyTo<T>(this IEnumerable<T> sequence, T[] array, int a
Debug.Assert(array != null);
Debug.Assert(arrayIndex >= 0 && arrayIndex <= array.Length);

// IList is the GCD of what the following types implement.
if (sequence is IList<T>)
// Optimized paths for well-known types, all other ICollection<T> implementations use the generic CopyTo fallback.
if (sequence is ICollection<T> collection)
{
Comment thread
prozolic marked this conversation as resolved.
if (sequence is List<T> list)
if (collection is List<T> list)
Copy link
Copy Markdown
Member

@tannergooding tannergooding Apr 12, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@prozolic prozolic Apr 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@jkotas jkotas Apr 13, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@prozolic prozolic Apr 15, 2026

Choose a reason for hiding this comment

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

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();
        }
    }

{
list.CopyTo(array, arrayIndex);
return true;
Expand All @@ -114,18 +114,31 @@ internal static bool TryCopyTo<T>(this IEnumerable<T> sequence, T[] array, int a
// Array.Copy can throw an ArrayTypeMismatchException if the underlying type of
// the destination array is not typeof(T[]), but is assignment-compatible with T[].
// See https://github.com/dotnet/runtime/issues/14794 for more info.
if (sequence.GetType() == typeof(T[]))
if (collection.GetType() == typeof(T[]))
{
var sourceArray = (T[])sequence;
var sourceArray = (T[])collection;
Array.Copy(sourceArray, 0, array, arrayIndex, sourceArray.Length);
return true;
}

if (sequence is ImmutableArray<T> immutable)
if (collection is ImmutableArray<T> immutable)
{
Array.Copy(immutable.array!, 0, array, arrayIndex, immutable.Length);
return true;
}

#if !NET
// On .NET Framework, if 'sequence' is actually a covariant array (for example, a string[] assigned to object[] and passed as ICollection<object>),
// sequence.GetType() won't match typeof(T[]), so the fast path above is skipped.
// The array's ICollection<T>.CopyTo implementation may call Array.Copy and throw an ArrayTypeMismatchException when copying into a T[].
if (collection is Array)
{
return false;
}
Comment thread
prozolic marked this conversation as resolved.
#endif

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

return false;
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Collections.Immutable/tests/ImmutableArrayTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1438,6 +1438,8 @@ public static IEnumerable<object[]> InsertRangeData()
yield return new object[] { s_empty, 0, new uint[] { 1, 2, 3 } };
yield return new object[] { s_manyElements, 0, new uint[] { 4, 5, 6 } };
yield return new object[] { s_manyElements, 3, new uint[] { 4, 5, 6 } };
yield return new object[] { s_manyElements, 0, new Dictionary<int, int> { [4] = 0, [5] = 0 }.Keys };
yield return new object[] { s_manyElements, 3, new Dictionary<int, int> { [4] = 0, [5] = 0 }.Keys };
}

[Theory]
Expand Down
Loading