Skip to content
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.

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

API proposal: Array.Clear(Array) #51581

Closed
GrabYourPitchforks opened this issue Apr 20, 2021 · 7 comments · Fixed by #53388
Closed

API proposal: Array.Clear(Array) #51581

GrabYourPitchforks opened this issue Apr 20, 2021 · 7 comments · Fixed by #53388
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Apr 20, 2021

In #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 #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 #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.

@GrabYourPitchforks GrabYourPitchforks added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime labels Apr 20, 2021
@GrabYourPitchforks GrabYourPitchforks added this to the 6.0.0 milestone Apr 20, 2021
@ghost
Copy link

ghost commented Apr 20, 2021

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

Issue Details

In #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 #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 #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 bounds check, I suppose.

Author: GrabYourPitchforks
Assignees: -
Labels:

api-suggestion, area-System.Runtime

Milestone: 6.0.0

@FilipToth
Copy link
Contributor

FilipToth commented Apr 21, 2021

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
Copy link

KalleOlaviNiemitalo commented Apr 21, 2021

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
Copy link
Member Author

@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
Copy link

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).

void IList.Clear()
{
Array.Clear(this, this.GetLowerBound(0), this.Length);
}

In contrast, the IList<T>.Clear() implementation in SZArrayHelper does throw NotSupportedException.

private void Clear<T>()
{
// ! Warning: "this" is an array, not an SZArrayHelper. See comments above
// ! or you may introduce a security hole!
ThrowHelper.ThrowNotSupportedException(ExceptionResource.NotSupported_ReadOnlyCollection);
}

@GrabYourPitchforks
Copy link
Member Author

BTW we just added the internal implementation of this method (see #51548), so at least IList.Clear() works for large multi-dim arrays now.

@GrabYourPitchforks GrabYourPitchforks added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 21, 2021
@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label May 22, 2021
@bartonjs
Copy link
Member

bartonjs commented May 27, 2021

Video

Looks good as proposed

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

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 27, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 27, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 28, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants