Use `Span<T>.Fill` implementation for `Array.Fill` #52590

xtqqczze · 2021-05-11T11:18:39Z

Fix #7049

Follow-up #51365

cc: @GrabYourPitchforks

ghost · 2021-05-11T11:18:47Z

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

Issue Details

Fix #7049

Follow-up #51365

Author:	xtqqczze
Assignees:	-
Labels:	`area-System.Runtime`
Milestone:	-

src/libraries/System.Private.CoreLib/src/System/Array.cs

GrabYourPitchforks

Creating a Span<T> wrapped around a T[] in this manner is illegal. (It doesn't account for array variance and could lead to type-safety violations.) You need to have logic similar to what's already in the Span<T> ctor:

runtime/src/libraries/System.Private.CoreLib/src/System/Span.cs

Lines 44 to 45 in b0c7f04

    
           if (!typeof(T).IsValueType && array.GetType() != typeof(T[])) 
        
               ThrowHelper.ThrowArrayTypeMismatchException();

Basically, if the span ctor would've succeeded, then turn it into a span and call fill. Otherwise loop and set each array element individually.

xtqqczze · 2021-05-11T23:55:24Z

Creating a Span<T> wrapped around a T[] in this manner is illegal.

I will make the suggested changes. However. I see the same pattern used elsewhere, without the type check, e.g.:

runtime/src/libraries/System.Private.CoreLib/src/System/Array.cs

Line 1868 in fd2e564

    
           var span = new Span<T>(ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(array), index), length);

xtqqczze · 2021-05-12T01:24:21Z

@gfoidl Could we combine conditions and just throw ArgumentOutOfRangeException, like:

runtime/src/libraries/System.Private.CoreLib/src/System/Span.cs

Lines 406 to 418 in fbd3b98

    
           #if TARGET_64BIT 
        
                       // Since start and length are both 32-bit, their sum can be computed across a 64-bit domain 
        
                       // without loss of fidelity. The cast to uint before the cast to ulong ensures that the 
        
                       // extension from 32- to 64-bit is zero-extending rather than sign-extending. The end result 
        
                       // of this is that if either input is negative or if the input sum overflows past Int32.MaxValue, 
        
                       // that information is captured correctly in the comparison against the backing _length field. 
        
                       // We don't use this same mechanism in a 32-bit process due to the overhead of 64-bit arithmetic. 
        
                       if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length) 
        
                           ThrowHelper.ThrowArgumentOutOfRangeException(); 
        
           #else 
        
                       if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start)) 
        
                           ThrowHelper.ThrowArgumentOutOfRangeException(); 
        
           #endif

gfoidl · 2021-05-12T08:38:12Z

Could we combine conditions and just throw ArgumentOutOfRangeException, like:

I like the idea, but I fear this a breaking change as a different exception will be thrown. In the span-code it's just the ArgumentOutOfRangeException, whilst the array-code has a distinction between index and count.
I'd go with your idea, and to be "symmetric" with the span-code, but I'm not in the position to tell if this is tolerable or not. Someone from the team should decide here.

GrabYourPitchforks · 2021-05-12T17:05:46Z

Correct - it's generally not considered an acceptable breaking change to change or combine exceptions like that. The reason we could get away with it in Span<T> is because those were completely new code paths with no expectation of previous compat.

GrabYourPitchforks · 2021-05-12T17:11:39Z

We should add extra unit tests for this:

If I have a Bar[] that has been downcast to an object[], I can call Array.Fill<object>(..., new Bar()), and the call will succeed.
If I have a Bar[] that has been downcast to an object[], I can call Array.Fill<object>(..., new Foo()), the call will fail with ArrayTypeMismatchException (or whatever the exception is), and the array will retain its original contents.
If I have an int[] that has been sidecast to a uint[], I can call Array.Fill<uint>(..., 42), and the call will succeed.

GrabYourPitchforks · 2021-05-12T17:22:06Z

src/libraries/System.Private.CoreLib/src/System/Array.cs

            {
-                array[i] = value;
+                var span = new Span<T>(ref MemoryMarshal.GetArrayDataReference(array), array.Length);


Shouldn't need this; should be able to do new Span<T>(array).Fill(value);

Right, we've already checked array for null so the null check in Span(T[]? array) should be eliminated by the jit. I don't think we can do similar in same Fill<T>(T[] array, T value, int startIndex, int count) because the argument validation is more complex.

src/libraries/System.Private.CoreLib/src/System/Array.cs

GrabYourPitchforks · 2021-05-13T17:09:50Z

src/libraries/System.Runtime/tests/System/ArrayTests.cs

+        public static void Fill_Casting()
+        {
+            Bar[] barArray = new Bar[2];
+            Array.Fill<object>(barArray, new Bar());


Please validate that Array.Fill actually populated the array. Something like:

public class Bar { public string Value; } Bar[] barArray = new Bar[] { new Bar() { Value = "1" }, new Bar() { Value = "2" }, }; Array.Fill<object>(barArray, new Bar() { Value = "x" }); Assert.Equal("x", barArray[0].Value); Assert.Equal("x", barArray[1].Value);

This method should also test the other Fill overload. For instance: new Bar[5], with Fill(theArray, 1, 2), ensuring that only theArray[1] and theArray[2] were changed. There's no need to unit test that the bounds check is correct; I think there's already a unit test for that.

And we should have a test between uint[] <-> int[] as well, to ensure that we didn't inadvertently break that logic. For instance:

int[] ints = new int[] { 10, 20, 30, 40, 50 }; uint[] uints = (uint[])(object)ints; // do work via Array.Fill(uints, ...); Assert.Equal(new int[] { /* whatever the expected sequence is */ }, ints);

GrabYourPitchforks · 2021-05-13T17:10:21Z

src/libraries/System.Runtime/tests/System/ArrayTests.cs

+        [Fact]
+        public static void Fill_Casting_Invalid()
+        {
+            Bar[] barArray = new Bar[2];


Similar to above, but after the call to Assert.Throws this unit test should ensure that the contents of the array remain unchanged.

jkotas · 2021-05-15T09:57:53Z

src/libraries/System.Runtime/tests/System/ArrayTests.cs

+        }
+
+        [Fact]
+        public static void Fill_Casting()


Add similar tests coverage for the other Array.Fill overload. If I am reading the code correctly, it has a bug that you should be able to find by adding the tests.

bug fixedup in 07dd088

jeffhandley · 2021-06-11T23:28:03Z

@xtqqczze Do you think you'll be able to address the remaining feedback?

xtqqczze · 2021-06-13T06:14:50Z

@xtqqczze Do you think you'll be able to address the remaining feedback?

I'll be able to continue working on this in the w/c 2021-06-20.

jeffhandley · 2021-06-13T06:24:37Z

Cool; thanks! If it's merged in by July 12, it'll make .NET 6.0.

GrabYourPitchforks · 2021-06-18T21:04:48Z

src/libraries/System.Runtime/tests/System/ArrayTests.cs

+            Assert.Throws<ArrayTypeMismatchException>(() => Array.Fill<object>(barArray1, new Foo()));
+
+            Bar[] barArray2 = CreateBarArray();
+            try


Don't use try / catch in these tests, as it won't properly handle the failure condition. Instead, use:

Assert.Throws<ArrayTypeMismatchException>(() => ...); Assert.Equal(...);

These two lines will check that the callback throws, and it will also check that the array has not been mutated, as you are doing here.

See near the top of this file for some other examples of using Assert.Throws.

GrabYourPitchforks · 2021-06-18T21:08:10Z

src/libraries/System.Runtime/tests/System/ArrayTests.cs

+        {
+            Bar[] barArray1 = CreateBarArray();
+            Array.Fill<object>(barArray1, new Bar() { Value = "x" });
+            Assert.Equal(Enumerable.Repeat("x", barArray1.Length), barArray1.Select(e => e.Value));


Consider using new string[] { 'x', 'x', 'x', 'x' } here to match the pattern you have in line 4285 below. Removing the call to Enumerable.Repeat uncomplicates the test.

(Same comment for line 4289.)

I was originally considering parameterised the tests, this would require the second Fill_Casting test to be written something like:

runtime/src/libraries/System.Runtime/tests/System/ArrayTests.cs

Lines 4216 to 4223 in 8c90687

T[] before = source.Take(startIndex).ToArray();

T[] after = source.Skip(startIndex + count).ToArray();

Array.Fill(array, value, startIndex, count);

Assert.Equal(before, array.Take(startIndex));

Assert.Equal(Enumerable.Repeat(value, count), array.Skip(startIndex).Take(count));

Assert.Equal(after, array.Skip(startIndex + count));

But this seemed unnecessarily complicated 😋

xtqqczze · 2021-06-22T15:18:49Z

runtime failed: TF400898: An Internal Error Occurred. Activity Id: d86bb713-ad27-498f-8159-716f1fb2f1f9.

GrabYourPitchforks · 2021-06-22T15:35:44Z

FYI - there's no need to force push. In fact, doing so makes the PR more difficult for us to review.
Everything will be properly squashed anyway into a single commit when it's eventually merged.

GrabYourPitchforks · 2021-06-22T15:44:38Z

Also, the PR as of commit c48c69c seemed fine. Why change it to df45239e19ff03b43175bc24ebc0fbfc543682ce? Seems like that change introduced compilation errors and reverted some of the test cleanup you did.

xtqqczze · 2021-06-22T16:08:08Z

Also, the PR as of commit c48c69c seemed fine. Why change it to df45239? Seems like that change introduced compilation errors and reverted some of the test cleanup you did.

To clarify here, df45239e19ff03b43175bc24ebc0fbfc543682ce was the commit you approved (which contained compilation errors), c48c69c is the current revision (which was force pushed).

Sorry for the confusion, in future, I shall avoid force push. If it helps I can rebase back onto df45239e19ff03b43175bc24ebc0fbfc543682ce to present a linear history.

GrabYourPitchforks · 2021-06-22T16:15:54Z

Derp, swapping the two was my mistake. But it's a good case study of how force pushing makes reviewing more difficult: because we need to fetch the two commits individually and perform a manual diff against them offline, which presents opportunity for reviewers to err (as I had done here). In the future, if you stick to simple pushes, it'll mean that we can review using just the web UI here, which makes looking at diffs much simpler. Thanks! :)

GrabYourPitchforks · 2021-06-22T16:16:57Z

CI failures are caused by https://status.dev.azure.com/_event/246897938. We'll sit tight and wait for this to resolve itself.

GrabYourPitchforks · 2021-06-24T17:54:08Z

@xtqqczze Looks like 2 categories of errors: The first is that c48c69c should be reverted, and those lines should instead read Assert.Equal<object>(new int[] { /* ... */ }, uintArray). This is just a weird consequence of how xunit performs equality checking in the version we pull in. The code you wrote would work in the newest version of xunit, but we don't yet pull it in to the repo.

The second is that I think you need to implement Bar.Equals on your test class. Otherwise xunit doesn't know how to compare those Bar objects for equality.

I think we're getting close here! :)

This reverts commit c48c69c.

xtqqczze · 2021-06-24T19:28:15Z

c48c69c should be reverted, and those lines should instead read Assert.Equal<object>(new int[] { /* ... */ }, uintArray).

I think we need Assert.Equal(new int[] { /* ... */ }, (int[])(object)uintArray) instead.

Otherwise, I see error CS1503 because the Equal(String, String) overload is selected.

GrabYourPitchforks · 2021-06-24T21:23:34Z

PeriodTimer failures are a known issue (also tracked via #54543).

xtqqczze · 2021-06-29T07:52:50Z

Test failures:

System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_Cookies_Http3_Mock.GetAsyncWithBasicAuth_ReceiveSetCookie_CookieSent
System.Net.Http.Functional.Tests.SyncHttpHandler_HttpProtocolTests_Dribble.GetAsync_ResponseHasNormalLineEndings_Success

These appear to be tracked by #54778.

GrabYourPitchforks · 2021-06-29T22:01:11Z

I kicked off a new manual run: https://dev.azure.com/dnceng/public/_build/results?buildId=1212225&view=results

GrabYourPitchforks · 2021-06-30T04:02:39Z

Remaining test failures are #28949, #54042

GrabYourPitchforks · 2021-06-30T04:06:37Z

Thanks @xtqqczze!

xtqqczze · 2021-07-01T16:08:01Z

@GrabYourPitchforks We got there in the end! Thanks for all your help with this.

xtqqczze added 2 commits May 11, 2021 11:58

Optimise Array.Fill implementation

bb1d2ee

Use Array.Fill rather then calling AsSpan().Fill

d6577fc

dotnet-issue-labeler bot added the area-System.Runtime label May 11, 2021

gfoidl reviewed May 11, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Array.cs Outdated Show resolved Hide resolved

xtqqczze added 2 commits May 11, 2021 15:06

Fix unecessary sign extension

41d5b9a

Optimise argument validation with uint

a851f05

GrabYourPitchforks requested changes May 11, 2021

View reviewed changes

xtqqczze force-pushed the array-fill branch from 2578c66 to a14ff1b Compare May 12, 2021 01:16

GrabYourPitchforks reviewed May 12, 2021

View reviewed changes

xtqqczze force-pushed the array-fill branch from 3b85bc4 to c283441 Compare May 13, 2021 08:44

GrabYourPitchforks requested changes May 13, 2021

View reviewed changes

jkotas reviewed May 15, 2021

View reviewed changes

xtqqczze marked this pull request as draft June 18, 2021 16:56

xtqqczze added 5 commits June 18, 2021 18:03

Add array variance checks

f261e92

Use Span(T[]? array) ctor

07dd088

Add casting tests

a2cf547

fixup! 07dd088

f6fee90

Add additional tests

8c90687

xtqqczze force-pushed the array-fill branch from c283441 to 8c90687 Compare June 18, 2021 20:32

GrabYourPitchforks requested changes Jun 18, 2021

View reviewed changes

remove try/catch

5168dab

xtqqczze marked this pull request as ready for review June 22, 2021 12:50

xtqqczze closed this Jun 22, 2021

xtqqczze reopened this Jun 22, 2021

xtqqczze closed this Jun 24, 2021

xtqqczze reopened this Jun 24, 2021

xtqqczze added 3 commits June 24, 2021 19:55

Revert "Fix CS1503"

f2cfecd

This reverts commit c48c69c.

Fix Assert.Equal

c718716

Implement IEquatable<Bar>

015e50f

xtqqczze closed this Jun 25, 2021

xtqqczze reopened this Jun 25, 2021

runfoapp bot mentioned this pull request Jun 25, 2021

Test failure System.Threading.Tests.PeriodicTimerTests.WaitForNextTickAsync_CanceledWaitThenWaitAgain_Succeeds #54713

Closed

xtqqczze closed this Jun 27, 2021

xtqqczze reopened this Jun 27, 2021

GrabYourPitchforks merged commit 99f7bae into dotnet:main Jun 30, 2021

xtqqczze deleted the array-fill branch July 1, 2021 16:08

ghost locked as resolved and limited conversation to collaborators Jul 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `Span<T>.Fill` implementation for `Array.Fill` #52590

Use `Span<T>.Fill` implementation for `Array.Fill` #52590

xtqqczze commented May 11, 2021 •

edited

Loading

ghost commented May 11, 2021

GrabYourPitchforks left a comment •

edited

Loading

xtqqczze commented May 11, 2021

xtqqczze commented May 12, 2021

gfoidl commented May 12, 2021

GrabYourPitchforks commented May 12, 2021

GrabYourPitchforks commented May 12, 2021

GrabYourPitchforks May 12, 2021

xtqqczze May 13, 2021

GrabYourPitchforks May 13, 2021

GrabYourPitchforks May 13, 2021

jkotas May 15, 2021

xtqqczze Jun 18, 2021

jeffhandley commented Jun 11, 2021

xtqqczze commented Jun 13, 2021

jeffhandley commented Jun 13, 2021

GrabYourPitchforks Jun 18, 2021

GrabYourPitchforks Jun 18, 2021

xtqqczze Jun 19, 2021

xtqqczze commented Jun 22, 2021

GrabYourPitchforks commented Jun 22, 2021

GrabYourPitchforks commented Jun 22, 2021

xtqqczze commented Jun 22, 2021

GrabYourPitchforks commented Jun 22, 2021

GrabYourPitchforks commented Jun 22, 2021

GrabYourPitchforks commented Jun 24, 2021

xtqqczze commented Jun 24, 2021

GrabYourPitchforks commented Jun 24, 2021

xtqqczze commented Jun 29, 2021

GrabYourPitchforks commented Jun 29, 2021

GrabYourPitchforks commented Jun 30, 2021

GrabYourPitchforks commented Jun 30, 2021

xtqqczze commented Jul 1, 2021

	if (!typeof(T).IsValueType && array.GetType() != typeof(T[]))
	ThrowHelper.ThrowArrayTypeMismatchException();

	T[] before = source.Take(startIndex).ToArray();
	T[] after = source.Skip(startIndex + count).ToArray();

	Array.Fill(array, value, startIndex, count);

	Assert.Equal(before, array.Take(startIndex));
	Assert.Equal(Enumerable.Repeat(value, count), array.Skip(startIndex).Take(count));
	Assert.Equal(after, array.Skip(startIndex + count));

Use Span<T>.Fill implementation for Array.Fill #52590

Use Span<T>.Fill implementation for Array.Fill #52590

Conversation

xtqqczze commented May 11, 2021 • edited Loading

ghost commented May 11, 2021

GrabYourPitchforks left a comment • edited Loading

Choose a reason for hiding this comment

xtqqczze commented May 11, 2021

xtqqczze commented May 12, 2021

gfoidl commented May 12, 2021

GrabYourPitchforks commented May 12, 2021

GrabYourPitchforks commented May 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffhandley commented Jun 11, 2021

xtqqczze commented Jun 13, 2021

jeffhandley commented Jun 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtqqczze commented Jun 22, 2021

GrabYourPitchforks commented Jun 22, 2021

GrabYourPitchforks commented Jun 22, 2021

xtqqczze commented Jun 22, 2021

GrabYourPitchforks commented Jun 22, 2021

GrabYourPitchforks commented Jun 22, 2021

GrabYourPitchforks commented Jun 24, 2021

xtqqczze commented Jun 24, 2021

GrabYourPitchforks commented Jun 24, 2021

xtqqczze commented Jun 29, 2021

GrabYourPitchforks commented Jun 29, 2021

GrabYourPitchforks commented Jun 30, 2021

GrabYourPitchforks commented Jun 30, 2021

xtqqczze commented Jul 1, 2021

Use `Span<T>.Fill` implementation for `Array.Fill` #52590

Use `Span<T>.Fill` implementation for `Array.Fill` #52590

xtqqczze commented May 11, 2021 •

edited

Loading

GrabYourPitchforks left a comment •

edited

Loading