Elide two redundant checks in implementation of ICollection.CopyTo(Array, int) in common collection types

[Enderlook]

Some collections when implementing ICollection.CopyTo(Array, int) has some redundant checks that could be elided.

These collections use the following method to copy their content:

runtime/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

Line 57 in 3f228d8

public static unsafe void Copy(Array sourceArray, int sourceIndex, Array destinationArray, int destinationIndex, int length)

Which checks here if both arrays are same type and single-dimensional:

runtime/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

Lines 61 to 63 in 3f228d8

	MethodTable* pMT = RuntimeHelpers.GetMethodTable(sourceArray);
	if (pMT == RuntimeHelpers.GetMethodTable(destinationArray) &&
	!pMT->IsMultiDimensionalArray &&

Or forwards the call to:

runtime/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

Line 84 in 3f228d8

Copy(sourceArray!, sourceIndex, destinationArray!, destinationIndex, length, reliable: false);

Which checks here that boths arrays must match same dimensions count:

runtime/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

Lines 94 to 95 in 3f228d8

	if (sourceArray.GetType() != destinationArray.GetType() && sourceArray.Rank != destinationArray.Rank)
	throw new RankException(SR.Rank_MustMatch);

And we already know that these collections use single-dimensional arrays. So we can take advantage of this check to elide our current ones.
Luckly, the collections that I'm talking right now, already use a try/catch pattern to check if array types mismatch. So adding another catch clause to the already existing try/catch won't degradate inlining (as methods with exception handling can't be inlined anyways).
This would save two checks when doing calls to these rare methods... which is not much but is an honest effort :)

List<T>

        void ICollection.CopyTo(Array array, int arrayIndex)
        {
-           if ((array != null) && (array.Rank != 1))
-           {
-               ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankMultiDimNotSupported);
-           }


            try
            {
                // Array.Copy will check for NULL.
                Array.Copy(_items, 0, array!, arrayIndex, _size);
            }
            catch (ArrayTypeMismatchException)
            {
                ThrowHelper.ThrowArgumentException_Argument_IncompatibleArrayType();
            }
+           catch (RankException)
+           {
+               ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankMultiDimNotSupported);
+           }
        }

From:

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/List.cs

Lines 387 to 403 in 3f228d8

	void ICollection.CopyTo(Array array, int arrayIndex)
	{
	if ((array != null) && (array.Rank != 1))
	{
	ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankMultiDimNotSupported);
	}
	try
	{
	// Array.Copy will check for NULL.
	Array.Copy(_items, 0, array!, arrayIndex, _size);
	}
	catch (ArrayTypeMismatchException)
	{
	ThrowHelper.ThrowArgumentException_Argument_IncompatibleArrayType();
	}
	}

Queue<T>

        void ICollection.CopyTo(Array array, int index)
        {
            ArgumentNullException.ThrowIfNull(array);


-           if (array.Rank != 1)
-           {
-               throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
-           }


            if (array.GetLowerBound(0) != 0)
            {
                throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
            }


            int arrayLen = array.Length;
            if (index < 0 || index > arrayLen)
            {
                throw new ArgumentOutOfRangeException(nameof(index), index, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
            }


            if (arrayLen - index < _size)
            {
                throw new ArgumentException(SR.Argument_InvalidOffLen);
            }


            int numToCopy = _size;
            if (numToCopy == 0) return;


            try
            {
                int firstPart = (_array.Length - _head < numToCopy) ? _array.Length - _head : numToCopy;
                Array.Copy(_array, _head, array, index, firstPart);
                numToCopy -= firstPart;


                if (numToCopy > 0)
                {
                    Array.Copy(_array, 0, array, index + _array.Length - _head, numToCopy);
                }
            }
            catch (ArrayTypeMismatchException)
            {
                throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
            }
+           catch (RankException)
+           {
+               throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
           }
        }

From:

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Queue.cs

Lines 125 to 168 in 3f228d8

	void ICollection.CopyTo(Array array, int index)
	{
	ArgumentNullException.ThrowIfNull(array);
	if (array.Rank != 1)
	{
	throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
	}
	if (array.GetLowerBound(0) != 0)
	{
	throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
	}
	int arrayLen = array.Length;
	if (index < 0 || index > arrayLen)
	{
	throw new ArgumentOutOfRangeException(nameof(index), index, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
	}
	if (arrayLen - index < _size)
	{
	throw new ArgumentException(SR.Argument_InvalidOffLen);
	}
	int numToCopy = _size;
	if (numToCopy == 0) return;
	try
	{
	int firstPart = (_array.Length - _head < numToCopy) ? _array.Length - _head : numToCopy;
	Array.Copy(_array, _head, array, index, firstPart);
	numToCopy -= firstPart;
	if (numToCopy > 0)
	{
	Array.Copy(_array, 0, array, index + _array.Length - _head, numToCopy);
	}
	}
	catch (ArrayTypeMismatchException)
	{
	throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
	}
	}

Stack<T>

        void ICollection.CopyTo(Array array, int arrayIndex)
        {
            ArgumentNullException.ThrowIfNull(array);


-           if (array.Rank != 1)
-           {
-               throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
-           }


            if (array.GetLowerBound(0) != 0)
            {
                throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
            }


            if (arrayIndex < 0 || arrayIndex > array.Length)
            {
                throw new ArgumentOutOfRangeException(nameof(arrayIndex), arrayIndex, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
            }


            if (array.Length - arrayIndex < _size)
            {
                throw new ArgumentException(SR.Argument_InvalidOffLen);
            }


            try
            {
                Array.Copy(_array, 0, array, arrayIndex, _size);
                Array.Reverse(array, arrayIndex, _size);
            }
            catch (ArrayTypeMismatchException)
            {
                throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
            }
+           catch (RankException)
+           {
+                throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
+           }
        }

From:

runtime/src/libraries/System.Collections/src/System/Collections/Generic/Stack.cs

Lines 119 to 152 in 3f228d8

	void ICollection.CopyTo(Array array, int arrayIndex)
	{
	ArgumentNullException.ThrowIfNull(array);
	if (array.Rank != 1)
	{
	throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
	}
	if (array.GetLowerBound(0) != 0)
	{
	throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
	}
	if (arrayIndex < 0 || arrayIndex > array.Length)
	{
	throw new ArgumentOutOfRangeException(nameof(arrayIndex), arrayIndex, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
	}
	if (array.Length - arrayIndex < _size)
	{
	throw new ArgumentException(SR.Argument_InvalidOffLen);
	}
	try
	{
	Array.Copy(_array, 0, array, arrayIndex, _size);
	Array.Reverse(array, arrayIndex, _size);
	}
	catch (ArrayTypeMismatchException)
	{
	throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
	}
	}

PriorityQueue<T>.UnorderedItemsCollection

            void ICollection.CopyTo(Array array, int index)
            {
                ArgumentNullException.ThrowIfNull(array);


-               if (array.Rank != 1)
-               {
-                   throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
-               }


                if (array.GetLowerBound(0) != 0)
                {
                    throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
                }


                if (index < 0 || index > array.Length)
                {
                    throw new ArgumentOutOfRangeException(nameof(index), index, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
                }


                if (array.Length - index < _queue._size)
                {
                    throw new ArgumentException(SR.Argument_InvalidOffLen);
                }


                try
                {
                    Array.Copy(_queue._nodes, 0, array, index, _queue._size);
                }
                catch (ArrayTypeMismatchException)
                {
                    throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
                }
+               catch (RankException)
+               {
+                   throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
+               }
            }

From:

runtime/src/libraries/System.Collections/src/System/Collections/Generic/PriorityQueue.cs

Lines 852 to 884 in 3f228d8

	void ICollection.CopyTo(Array array, int index)
	{
	ArgumentNullException.ThrowIfNull(array);
	if (array.Rank != 1)
	{
	throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
	}
	if (array.GetLowerBound(0) != 0)
	{
	throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
	}
	if (index < 0 || index > array.Length)
	{
	throw new ArgumentOutOfRangeException(nameof(index), index, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
	}
	if (array.Length - index < _queue._size)
	{
	throw new ArgumentException(SR.Argument_InvalidOffLen);
	}
	try
	{
	Array.Copy(_queue._nodes, 0, array, index, _queue._size);
	}
	catch (ArrayTypeMismatchException)
	{
	throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
	}
	}

By the way, while reading about these, I found that some collections uses:

 throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));

While others use:

ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankMultiDimNotSupported);

Shouldn't the ones that don't use but have access to that (internal) helper -such as Queue<T>- use it?

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

Issue Details

---

Some collections when implementing ICollection.CopyTo(Array, int) has some redundant checks that could be elided.

These collections use the following method to copy their content:

runtime/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

Line 57 in 3f228d8

public static unsafe void Copy(Array sourceArray, int sourceIndex, Array destinationArray, int destinationIndex, int length)

Which checks here if both arrays are same type and single-dimensional:

runtime/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

Lines 61 to 63 in 3f228d8

	MethodTable* pMT = RuntimeHelpers.GetMethodTable(sourceArray);
	if (pMT == RuntimeHelpers.GetMethodTable(destinationArray) &&
	!pMT->IsMultiDimensionalArray &&

Or forwards the call to:

runtime/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

Line 84 in 3f228d8

Copy(sourceArray!, sourceIndex, destinationArray!, destinationIndex, length, reliable: false);

Which checks here that boths arrays must match same dimensions count:

runtime/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

Lines 94 to 95 in 3f228d8

	if (sourceArray.GetType() != destinationArray.GetType() && sourceArray.Rank != destinationArray.Rank)
	throw new RankException(SR.Rank_MustMatch);

And we already know that these collections use single-dimensional arrays. So we can take advantage of this check to elide our current ones.
Luckly, the collections that I'm talking right now, already use a try/catch pattern to check if array types mismatch. So adding another catch clause to the already existing try/catch won't degradate inlining (as methods with exception handling can't be inlined anyways).
This would save two checks when doing calls to these rare methods... which is not much but is an honest effort :)

List<T>

        void ICollection.CopyTo(Array array, int arrayIndex)
        {
-           if ((array != null) && (array.Rank != 1))
-           {
-               ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankMultiDimNotSupported);
-           }


            try
            {
                // Array.Copy will check for NULL.
                Array.Copy(_items, 0, array!, arrayIndex, _size);
            }
            catch (ArrayTypeMismatchException)
            {
                ThrowHelper.ThrowArgumentException_Argument_IncompatibleArrayType();
            }
+           catch (RankException)
+           {
+               ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankMultiDimNotSupported);
+           }
        }

From:

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/List.cs

Lines 387 to 403 in 3f228d8

	void ICollection.CopyTo(Array array, int arrayIndex)
	{
	if ((array != null) && (array.Rank != 1))
	{
	ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankMultiDimNotSupported);
	}
	try
	{
	// Array.Copy will check for NULL.
	Array.Copy(_items, 0, array!, arrayIndex, _size);
	}
	catch (ArrayTypeMismatchException)
	{
	ThrowHelper.ThrowArgumentException_Argument_IncompatibleArrayType();
	}
	}

Queue<T>

        void ICollection.CopyTo(Array array, int index)
        {
            ArgumentNullException.ThrowIfNull(array);


-           if (array.Rank != 1)
-           {
-               throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
-           }


            if (array.GetLowerBound(0) != 0)
            {
                throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
            }


            int arrayLen = array.Length;
            if (index < 0 || index > arrayLen)
            {
                throw new ArgumentOutOfRangeException(nameof(index), index, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
            }


            if (arrayLen - index < _size)
            {
                throw new ArgumentException(SR.Argument_InvalidOffLen);
            }


            int numToCopy = _size;
            if (numToCopy == 0) return;


            try
            {
                int firstPart = (_array.Length - _head < numToCopy) ? _array.Length - _head : numToCopy;
                Array.Copy(_array, _head, array, index, firstPart);
                numToCopy -= firstPart;


                if (numToCopy > 0)
                {
                    Array.Copy(_array, 0, array, index + _array.Length - _head, numToCopy);
                }
            }
            catch (ArrayTypeMismatchException)
            {
                throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
            }
+           catch (RankException)
+           {
+               throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
           }
        }

From:

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Queue.cs

Lines 125 to 168 in 3f228d8

	void ICollection.CopyTo(Array array, int index)
	{
	ArgumentNullException.ThrowIfNull(array);
	if (array.Rank != 1)
	{
	throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
	}
	if (array.GetLowerBound(0) != 0)
	{
	throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
	}
	int arrayLen = array.Length;
	if (index < 0 || index > arrayLen)
	{
	throw new ArgumentOutOfRangeException(nameof(index), index, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
	}
	if (arrayLen - index < _size)
	{
	throw new ArgumentException(SR.Argument_InvalidOffLen);
	}
	int numToCopy = _size;
	if (numToCopy == 0) return;
	try
	{
	int firstPart = (_array.Length - _head < numToCopy) ? _array.Length - _head : numToCopy;
	Array.Copy(_array, _head, array, index, firstPart);
	numToCopy -= firstPart;
	if (numToCopy > 0)
	{
	Array.Copy(_array, 0, array, index + _array.Length - _head, numToCopy);
	}
	}
	catch (ArrayTypeMismatchException)
	{
	throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
	}
	}

Stack<T>

        void ICollection.CopyTo(Array array, int arrayIndex)
        {
            ArgumentNullException.ThrowIfNull(array);


-           if (array.Rank != 1)
-           {
-               throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
-           }


            if (array.GetLowerBound(0) != 0)
            {
                throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
            }


            if (arrayIndex < 0 || arrayIndex > array.Length)
            {
                throw new ArgumentOutOfRangeException(nameof(arrayIndex), arrayIndex, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
            }


            if (array.Length - arrayIndex < _size)
            {
                throw new ArgumentException(SR.Argument_InvalidOffLen);
            }


            try
            {
                Array.Copy(_array, 0, array, arrayIndex, _size);
                Array.Reverse(array, arrayIndex, _size);
            }
            catch (ArrayTypeMismatchException)
            {
                throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
            }
+           catch (RankException)
+           {
+                throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
+           }
        }

From:

runtime/src/libraries/System.Collections/src/System/Collections/Generic/Stack.cs

Lines 119 to 152 in 3f228d8

	void ICollection.CopyTo(Array array, int arrayIndex)
	{
	ArgumentNullException.ThrowIfNull(array);
	if (array.Rank != 1)
	{
	throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
	}
	if (array.GetLowerBound(0) != 0)
	{
	throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
	}
	if (arrayIndex < 0 || arrayIndex > array.Length)
	{
	throw new ArgumentOutOfRangeException(nameof(arrayIndex), arrayIndex, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
	}
	if (array.Length - arrayIndex < _size)
	{
	throw new ArgumentException(SR.Argument_InvalidOffLen);
	}
	try
	{
	Array.Copy(_array, 0, array, arrayIndex, _size);
	Array.Reverse(array, arrayIndex, _size);
	}
	catch (ArrayTypeMismatchException)
	{
	throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
	}
	}

PriorityQueue<T>.UnorderedItemsCollection

            void ICollection.CopyTo(Array array, int index)
            {
                ArgumentNullException.ThrowIfNull(array);


-               if (array.Rank != 1)
-               {
-                   throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
-               }


                if (array.GetLowerBound(0) != 0)
                {
                    throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
                }


                if (index < 0 || index > array.Length)
                {
                    throw new ArgumentOutOfRangeException(nameof(index), index, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
                }


                if (array.Length - index < _queue._size)
                {
                    throw new ArgumentException(SR.Argument_InvalidOffLen);
                }


                try
                {
                    Array.Copy(_queue._nodes, 0, array, index, _queue._size);
                }
                catch (ArrayTypeMismatchException)
                {
                    throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
                }
+               catch (RankException)
+               {
+                   throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
+               }
            }

From:

runtime/src/libraries/System.Collections/src/System/Collections/Generic/PriorityQueue.cs

Lines 852 to 884 in 3f228d8

	void ICollection.CopyTo(Array array, int index)
	{
	ArgumentNullException.ThrowIfNull(array);
	if (array.Rank != 1)
	{
	throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));
	}
	if (array.GetLowerBound(0) != 0)
	{
	throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array));
	}
	if (index < 0 || index > array.Length)
	{
	throw new ArgumentOutOfRangeException(nameof(index), index, SR.ArgumentOutOfRange_IndexMustBeLessOrEqual);
	}
	if (array.Length - index < _queue._size)
	{
	throw new ArgumentException(SR.Argument_InvalidOffLen);
	}
	try
	{
	Array.Copy(_queue._nodes, 0, array, index, _queue._size);
	}
	catch (ArrayTypeMismatchException)
	{
	throw new ArgumentException(SR.Argument_IncompatibleArrayType, nameof(array));
	}
	}

By the way, while reading about these, I found that some collections uses:

 throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array));

While others use:

ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankMultiDimNotSupported);

Shouldn't the ones that don't use but have access to that (internal) helper -such as Queue<T>- use it?

Author:	Enderlook
Assignees:	-
Labels:	area-System.Collections, untriaged
Milestone:	-

[eiriktsarpalis]

I generally prefer running validation and throwing early rather than handling exceptions, but that's just me. Are there any measurable performance benefits to doing this?

Shouldn't the ones that don't use but have access to that (internal) helper -such as Queue- use it?

Yes, they probably should.