Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position #23817

OmarTawfik · 2017-09-05T23:57:06Z

Fixes #23680
Replaces #23730

stephentoub · 2017-09-06T14:00:35Z

src/System.Linq/tests/ConcatTests.cs

@@ -421,5 +421,18 @@ public void GetEnumerableOfConcatCollectionChainFollowedByEnumerableNodeShouldBe
                Assert.Equal(0xf00, en.Current);
            }
        }
+
+        [Fact]
+        public void CollectionInterleavedWithLazyEnumerables_ToArray_Regression()


Nit: Do we need the "_Regression" suffix on these tests? In a year it won't matter to someone reading the code whether we added a test proactively or in response to a bug.

@stephentoub I personally added _Regression because someone might think "what a weird test. What is this supposed to cover?" / etc. in the future. Admittedly, it might be more helpful to remove the suffix and just link to the github issue in a comment:

public void CollectionInterleavedWithLazyEnumerables_ToArray() { // This is a regression test for https://github.com/dotnet/corefx/issues/23680 ... }

what a weird test. What is this supposed to cover?

That's why test naming is important, to qualify what the test is for. Comments are good, too, of course. But "_Regression" as a suffix is so moment-in-time. If you were to follow true TDD, for example, everything would need to be suffixed with "_Regression" because the tests are all written before any code is and thus all fail initially.

I'll add a comment with the bug URL.

stephentoub · 2017-09-06T14:19:52Z

src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs

+            {
+                buffer = GetBuffer(++row);
+                copied = CopyToCore(buffer, 0);
+            }


We don't have any tests that enter this loop.

Added tests with larger data sets

stephentoub · 2017-09-06T14:20:21Z

src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs

-                // end of the current buffer.
-                int copyCount = Math.Min(buffer.Length, count);
+                // Copy until we satisfy `count` or reach the end of the current buffer.
+                int copyCount = Math.Min(sourceBuffer.Length - sourceIndex, count);

                if (copyCount > 0)


We don't have any tests where copyCount == 0.

@OmarTawfik I don't think this can happen anymore. Add these asserts and remove the conditional:

+Debug.Assert(count > 0); Debug.Assert(sourceBuffer.Length > sourceIndex); // Copy until we satisfy `count` or reach the end of the current buffer. int copyCount = Math.Min(sourceBuffer.Length - sourceIndex, count); +Debug.Assert(copyCount > 0);

stephentoub · 2017-09-06T14:21:37Z

src/System.Linq/tests/SelectManyTests.cs

+        public void CollectionInterleavedWithLazyEnumerables_ToArray_Regression()
+        {
+            var results = new[] { 4, 5, 6 }
+                .SelectMany(i => i == 5 ?


Seems like we should have tests with other operators and with varying sizes, not just SelectMany/Concat and not just with one or a few items.

Also, since various operators specialize on the source collection type, seems like we need tests on various source types.

not just SelectMany/Concat

I explained in my last PR's description that since Append/Prepend never use the incorrect returned CopyPosition, they shouldn't be affected by this bug.

I explained in my last PR's description that since Append/Prepend never use the incorrect returned CopyPosition, they shouldn't be affected by this bug.

Even if that's true of the current implementation, you're assuming the current implementation will never change. Tests are important both to verify existing behavior and to help prevent future regressions.

The bug is affecting only SelectMany() and Concat(). I've added tests for small and large data sets.

jamesqo · 2017-09-06T19:06:54Z

src/Common/src/System/Collections/Generic/SparseArrayBuilder.cs

@@ -149,8 +149,11 @@ public void CopyTo(T[] array, int arrayIndex, int count)
                count -= reservedCount;
            }

-            // Finish copying after the final marker.
-            _builder.CopyTo(position, array, arrayIndex, count);
+            if (count > 0)


This is beneficial for scenarios when we insert a marker at the very end, e.g. nonCollection.Append().Append()./* a bunch of times */ or nonCollection.Concat(collection), so 👍

jamesqo · 2017-09-06T19:08:00Z

src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs

+                ? new CopyPosition(row + 1, 0)
+                : new CopyPosition(row, copied);
+
+            int CopyToCore(T[] sourceBuffer, int sourceIndex)


Nit: Is it customary in corefx to put a local func at the very end of the containing method?

We have very few local methods; there's no convention yet. But unless there's an obviously better place, end of the method makes sense.

I'm not sure what is the custom for this repo. I've not seen a lot of local functions used here yet. But that pattern is common.

jamesqo · 2017-09-06T19:09:47Z

src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs

            {
-                T[] buffer = GetBuffer(index: row);
+                Debug.Assert(sourceBuffer.Length > sourceIndex);


You mentioned previously this assert failed. It's good to see that's no longer the case.

Yes, because of the if (count > 0) check added to the calling method.

jamesqo · 2017-09-06T19:14:22Z

src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs

-                // end of the current buffer.
-                int copyCount = Math.Min(buffer.Length, count);
+                // Copy until we satisfy `count` or reach the end of the current buffer.
+                int copyCount = Math.Min(sourceBuffer.Length - sourceIndex, count);

                if (copyCount > 0)


@OmarTawfik I don't think this can happen anymore. Add these asserts and remove the conditional:

+Debug.Assert(count > 0); Debug.Assert(sourceBuffer.Length > sourceIndex); // Copy until we satisfy `count` or reach the end of the current buffer. int copyCount = Math.Min(sourceBuffer.Length - sourceIndex, count); +Debug.Assert(copyCount > 0);

jamesqo · 2017-09-06T19:19:51Z

src/System.Linq/tests/ConcatTests.cs

@@ -421,5 +421,18 @@ public void GetEnumerableOfConcatCollectionChainFollowedByEnumerableNodeShouldBe
                Assert.Equal(0xf00, en.Current);
            }
        }
+
+        [Fact]
+        public void CollectionInterleavedWithLazyEnumerables_ToArray_Regression()


@stephentoub I personally added _Regression because someone might think "what a weird test. What is this supposed to cover?" / etc. in the future. Admittedly, it might be more helpful to remove the suffix and just link to the github issue in a comment:

public void CollectionInterleavedWithLazyEnumerables_ToArray() { // This is a regression test for https://github.com/dotnet/corefx/issues/23680 ... }

jamesqo · 2017-09-06T19:22:25Z

src/System.Linq/tests/SelectManyTests.cs

+        public void CollectionInterleavedWithLazyEnumerables_ToArray_Regression()
+        {
+            var results = new[] { 4, 5, 6 }
+                .SelectMany(i => i == 5 ?


not just SelectMany/Concat

I explained in my last PR's description that since Append/Prepend never use the incorrect returned CopyPosition, they shouldn't be affected by this bug.

jamesqo · 2017-09-06T19:45:05Z

src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs

+
+            return copied == buffer.Length
+                ? new CopyPosition(row + 1, 0)
+                : new CopyPosition(row, copied);


Hmm, I believe the previous setup I had in place which looked like

/* first copy */ if (count == 0) { // Note it's `column + copied` instead of `copied` return new CopyPosition(row, column + copied).Normalize(buffer.Length); } do { /* subsequent copies */ } while (count > 0); return new CopyPosition(row, copied).Normalize(buffer.Length);

was correct. With the current code, something like

new TE<int>(new[] { 1 }).Concat(new TE<int>(new[] { 2 })).Concat(new TE<int>(new[] { 3 })).Concat(new[] { 4 }).ToArray()

or

new[] { 1, 2, 3, 4 }.SelectMany(i => i == 4 ? new[] { i } : new TE<int>(new[] { i })).ToArray()

should result in 1 2 2 4. Can you verify that's true?

If it is, you can add a regression test for them named NonCollectionThatDoesNotBeginAtStartOfRow_FollowedByOtherNonCollections. What happens is the LAB the non-collections are buffered into looks like this:

C0 C1 C2 R0: 1 2 3

The first LAB.CopyTo takes (row = 0, column = 0) as input, copies 1 item (1), and returns (0, 1). The 2nd LAB.CopyTo takes (0, 1) as input, copies 1 item (2) and returns (0, 1) when it should return (0, 2). The 3rd LAB.CopyTo then takes (0, 1) as input and copies 2 instead of 3.

The previous code is not needed because we now assert that count is never zero. However, I'll add the tests mentioned.

stephentoub · 2017-09-06T19:48:08Z

I have to ask: is all of this complexity and potential bug farm really worth it, vs just reverting #14675?
cc: @VSadov

jamesqo · 2017-09-06T20:01:21Z

@stephentoub I can see where you're coming from, and the practical part of me agrees with you. However, a part of me would be sad to see the code go... 😢 If you want, I can try cleaning up the code and renaming things after this PR to make it easier for other people to grasp what it does, to make any future bugs intuitive/obvious to fix for others. So far, although I've tried explaining it a lot, nobody seems to understand my code but me.

I am waiting to hear what @VSadov has to say though.

jamesqo · 2017-09-07T00:57:00Z

@OmarTawfik OK, I was wrong in my earlier comment when I said that

new TE<int> { 1 }.Concat(new TE<int> { 2 }).Concat(new TE<int> { 3 }).Concat(new[] { 4 }).ToArray()

would fail. I've debugged a little to see why. Here's a test case that actually does fail:

new TE<int> { 1 }.Concat(new[] { 2 }).Concat(new TE<int> { 3 }).Concat(new[] { 4 }).Concat(new TE<int> { 5 }).ToArray()
// Expected: 1 2 3 4 5
// Actual: 1 2 3 4 3

The fix is to go back to the previous setup in LAB.CopyTo which looked like

/* first copy */
if (count == 0)
{
    // Note it's `column + copied` instead of `copied`.
    return new CopyPosition(row, column + copied).Normalize(buffer.Length);
}

do
{
    /* subsequent copies */
}
while (count > 0);

return new CopyPosition(row, copied).Normalize(buffer.Length);

jamesqo · 2017-09-07T01:03:59Z

@OmarTawfik After you apply the above change to avoid the bug I pointed out, this PR looks fine to me. I have some minor nits but those can wait until a later PR.

VSadov · 2017-09-07T01:18:12Z

src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs

-                if (copyCount > 0)
-                {
-                    Array.Copy(buffer, column, array, arrayIndex, copyCount);
+            int CopyToCore(T[] sourceBuffer, int sourceIndex)


I think this could be simpler without a local function, but this will work too.

VSadov

LGTM

OmarTawfik · 2017-09-07T03:23:54Z

@jamesqo thanks for the additional test case!

stephentoub · 2017-09-07T18:45:00Z

src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs

+            T[] buffer = GetBuffer(row);
+            int copied = CopyToCore(buffer, column);
+
+            if(count == 0)


Nit: in case you end up making other edits, there's a missing space between if and (

Also, FWIW, I find it very confusing that CopyToCore is capturing count and modifying it. There's no indication at the call site that's happening, which means it looks like the only way count could be 0 is if it was passed in as 0, and yet there's a debug assert for that above.

stephentoub · 2017-09-07T18:54:49Z

src/System.Linq/tests/ConcatTests.cs

+        {
+            // See https://github.com/dotnet/corefx/issues/23680
+
+            var concats = arrays[0];


Nit: we try to use var only when the type is obvious from the right hand side, which it isn't here or below

stephentoub · 2017-09-07T19:15:03Z

If you believe this fully addresses the bug, with all corner cases covers, then LGTM (other than my comments).

OmarTawfik · 2017-09-07T19:25:58Z

@stephentoub addressed the nit and added a couple more cases. I believe this is ready. I'll merge this as soon as it is green and prepare a PR for the release branch.

OmarTawfik · 2017-09-07T20:55:28Z

@dotnet-bot test Tizen armel Debug Build

…#23865) * Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position (#23817) * Fix #23680 * PR Feedback * More tests * More tests * Not using a local function

…dotnet#23817) * Fix #23680 * PR Feedback * More tests * More tests

…dotnet/corefx#23817) * Fix dotnet/corefx#23680 * PR Feedback * More tests * More tests Commit migrated from dotnet/corefx@9b88997

Fix #23680

ffe169c

OmarTawfik added area-System.Linq bug Product bug (most likely) labels Sep 5, 2017

OmarTawfik self-assigned this Sep 5, 2017

OmarTawfik requested review from stephentoub, VSadov and jamesqo September 5, 2017 23:57

dnfclas added the cla-already-signed label Sep 5, 2017

stephentoub mentioned this pull request Sep 6, 2017

[WIP] Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position #23730

Closed

stephentoub reviewed Sep 6, 2017

View reviewed changes

jamesqo reviewed Sep 6, 2017

View reviewed changes

PR Feedback

1344a6c

VSadov reviewed Sep 7, 2017

View reviewed changes

VSadov approved these changes Sep 7, 2017

View reviewed changes

More tests

bcc79be

stephentoub reviewed Sep 7, 2017

View reviewed changes

More tests

ff2c794

OmarTawfik merged commit 9b88997 into dotnet:master Sep 7, 2017

OmarTawfik deleted the fix-to-array branch September 7, 2017 21:28

OmarTawfik mentioned this pull request Sep 7, 2017

Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position #23865

Merged

karelz added this to the 2.1.0 milestone Sep 7, 2017

marek-safar pushed a commit to mono/corefx that referenced this pull request Jan 19, 2018

Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position (…

fc70ae3

…dotnet#23817) * Fix #23680 * PR Feedback * More tests * More tests

Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position #23817

Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position #23817

Conversation

OmarTawfik commented Sep 5, 2017

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

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

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

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

jamesqo Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Sep 6, 2017

jamesqo commented Sep 6, 2017 • edited Loading

jamesqo commented Sep 7, 2017 • edited Loading

jamesqo commented Sep 7, 2017

Choose a reason for hiding this comment

VSadov left a comment

Choose a reason for hiding this comment

OmarTawfik commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Sep 7, 2017

OmarTawfik commented Sep 7, 2017

OmarTawfik commented Sep 7, 2017

jamesqo Sep 6, 2017 •

edited

Loading

jamesqo commented Sep 6, 2017 •

edited

Loading

jamesqo commented Sep 7, 2017 •

edited

Loading