dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.05k stars 4.69k forks source link

API proposal: Array.Clear(Array) #51581

Closed GrabYourPitchforks closed 3 years ago

GrabYourPitchforks commented 3 years ago

In https://github.com/dotnet/runtime/issues/18616 we discussed the idea of creating a generic method Array.Clear<T>(T[]) as an accelerator for clearing an array. However, there were a few reasons we decided against it, including the compiler potentially not matching arguments as expected, and the runtime incurring some amount of generic instantiation bloat.

However, per some discussion at https://github.com/dotnet/runtime/pull/51548, there could be a benefit to exposing a non-generic Array.Clear(Array) for callers to consume. This method would work both for standard SZArrays and for MDArrays, as discussed further below.

Proposed API

namespace System
{
    public class Array
    {
        // NEW PROPOSED SIGNATURE
        public static void Clear(Array array);

        // existing signature, for reference
        public static void Clear(Array array, int index, int length);
    }
}

Discussion

For SZArrays, the following calls would behave identically:

T[] myArray = GetArray();
Array.Clear(myArray, 0, myArray.Length);
Array.Clear(myArray);
myArray.Clear(); // (alternatively)

For MDArrays, there's no current API that can act as an accelerator. You'd need to write some complicated loop which iterates over the lower and upper bound for each rank, creates an appropriate int[] to represent an element's position, then calls Array.SetValue(null, int[]). The API proposed here would act as an appropriate accelerator for this scenario.

The implementation of Array.Clear(Array) doesn't provide a significant clock time improvement over the existing Array.Clear(Array, int, int) case. On my machine the difference was approximately 1 nanosecond per call. I'm guessing this was largely due to all of the "are the arguments out of bounds?" checks in the original method being predicted not-taken. However, it does present a small codegen improvement for callers (see discussion at top of https://github.com/dotnet/runtime/pull/51548), and IMO it makes the call sites easier to read: "clear the array" with no need to worry about bounds.

Ecosystem impact

Checking source.dot.net shows currently 182 references to the existing Array.Clear(Array, int, int) method. I estimate around half of these can be updated to call the new accelerator.

Alternative designs

We could make Clear an instance method instead of a static method. This matches APIs like GetValue and SetValue (instance methods), but not APIs like Clear or Fill or IndexOf (static methods). I don't have any immediate thoughts on whether a static or an instance method would ultimately be more discoverable. An instance method would allow us to elide a null arg check, I suppose.

ghost commented 3 years ago

Tagging subscribers to this area: @tannergooding See info in area-owners.md if you want to be subscribed.

Issue Details
In https://github.com/dotnet/runtime/issues/18616 we discussed the idea of creating a generic method `Array.Clear(T[])` as an accelerator for clearing an array. However, there were a few reasons we decided against it, including the compiler potentially not matching arguments as expected, and the runtime incurring some amount of generic instantiation bloat. However, per some discussion at https://github.com/dotnet/runtime/pull/51548, there could be a benefit to exposing a _non-generic_ `Array.Clear(Array)` for callers to consume. This method would work both for standard SZArrays and for MDArrays, as discussed further below. ## Proposed API ```cs namespace System { public class Array { // NEW PROPOSED SIGNATURE public static void Clear(Array array); // existing signature, for reference public static void Clear(Array array, int index, int length); } } ``` ## Discussion For SZArrays, the following calls would behave identically: ```cs T[] myArray = GetArray(); Array.Clear(myArray, 0, myArray.Length); Array.Clear(myArray); myArray.Clear(); // (alternatively) ``` For MDArrays, there's no current API that can act as an accelerator. You'd need to write some complicated loop which iterates over the lower and upper bound for each rank, creates an appropriate `int[]` to represent an element's position, then calls `Array.SetValue(null, int[])`. The API proposed here would act as an appropriate accelerator for this scenario. The implementation of `Array.Clear(Array)` doesn't provide a significant clock time improvement over the existing `Array.Clear(Array, int, int)` case. On my machine the difference was approximately 1 nanosecond per call. I'm guessing this was largely due to all of the "are the arguments out of bounds?" checks in the original method being predicted not-taken. However, it does present a small codegen improvement for callers (see discussion at top of https://github.com/dotnet/runtime/pull/51548), and IMO it makes the call sites easier to read: "clear the array" with no need to worry about bounds. ## Ecosystem impact Checking source.dot.net shows currently [__182 references__](https://source.dot.net/System.Private.CoreLib/R/bed27cecfb5f7a6c.html) to the existing `Array.Clear(Array, int, int)` method. I estimate around half of these can be updated to call the new accelerator. ## Alternative designs We could make `Clear` an instance method instead of a static method. This matches APIs like `GetValue` and `SetValue` (instance methods), but not APIs like `Clear` or `Fill` or `IndexOf` (static methods). I don't have any immediate thoughts on whether a static or an instance method would ultimately be more discoverable. An instance method would allow us to elide a bounds check, I suppose.
Author: GrabYourPitchforks
Assignees: -
Labels: `api-suggestion`, `area-System.Runtime`
Milestone: 6.0.0
FilipToth commented 3 years ago

I think creating an overload method for Array.Clear() with no parameters would an excellent idea.

I would also like to see this as a non-static method.

KalleOlaviNiemitalo commented 3 years ago

Array explicitly implements IList.Clear(). It might be a bit confusing if Array also had another Clear() instance method with different behavior. This probably wouldn't cause many actual bugs though, even in languages that cannot disambiguate the call, because the EII always throws and thus seems unlikely to be called intentionally. Anyway, a static Array.Clear(Array) method would avoid that issue altogether and be more consistent with the existing Array.Clear(Array, int, int).

GrabYourPitchforks commented 3 years ago

@KalleOlaviNiemitalo There are certain cases (large multidimensional arrays) where IList.Clear fails today, but where the Array.Clear method proposed here would have succeeded. The idea is that existing APIs like IList.Clear would be replumbed atop this.

KalleOlaviNiemitalo commented 3 years ago

Huh, I had the impression that the IList.Clear() implementation in Array would always throw, because IList.Clear() is documented as removing all elements and that cannot be done in a fixed-size array. But indeed it calls Array.Clear(Array, int, int).

https://github.com/dotnet/runtime/blob/2f740adc1457e8a28c1c072993b66f515977eb51/src/libraries/System.Private.CoreLib/src/System/Array.cs#L295-L298

In contrast, the IList\.Clear() implementation in SZArrayHelper does throw NotSupportedException.

https://github.com/dotnet/runtime/blob/f1a9ad1521966ea0a4330d34a2dd02a3744ce9fb/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs#L512-L518

GrabYourPitchforks commented 3 years ago

BTW we just added the internal implementation of this method (see https://github.com/dotnet/runtime/pull/51548), so at least IList.Clear() works for large multi-dim arrays now.

bartonjs commented 3 years ago

Video

Looks good as proposed

namespace System
{
    partial class Array
    {
        public static void Clear(Array array);
    }
}