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

Change BulkMoveWithWriteBarrier to be GC suspension friendly #13764

Closed
stephentoub opened this issue Nov 8, 2019 · 1 comment · Fixed by dotnet/coreclr#27776
Closed

Change BulkMoveWithWriteBarrier to be GC suspension friendly #13764

stephentoub opened this issue Nov 8, 2019 · 1 comment · Fixed by dotnet/coreclr#27776

Comments

@stephentoub
Copy link
Member

PR dotnet/coreclr#27758 reverted dotnet/coreclr@5e1ef69 because it caused a failure in a Sockets test (dotnet/corefx#42443 (comment), dotnet/corefx#42443 (comment)). We should investigate and revert the revert with the appropriate fix.

I extracted a repro that doesn't depend on sockets (it's possible it could be simplified further, didn't spend more time on it):

    [Theory]
    [InlineData(1024)]
    public unsafe void Test(int copyLength)
    {
        var buffer1 = new byte[2400];
        for (int i = 0; i < buffer1.Length; i++) buffer1[i] = (byte)(i % 256);

        var buffer2 = new byte[buffer1.Length];
        var segments = new List<ArraySegment<byte>>();
        for (int i = 0; i < buffer2.Length; i++) segments.Add(new ArraySegment<byte>(buffer2, i, 1));

        var buffer1Handles = new List<GCHandle>();
        var buffer2Handles = new List<GCHandle>();
        for (int i = 0; i < buffer1.Length; i++) buffer1Handles.Add(GCHandle.Alloc(buffer1, GCHandleType.Pinned));
        for (int i = 0; i < buffer2.Length; i++) buffer2Handles.Add(GCHandle.Alloc(buffer2, GCHandleType.Pinned));

        int count = 0;
        while (segments.Count > 0)
        {
            int i;
            for (i = 0; i < copyLength && count < buffer1.Length; i++)
            {
                *(byte*)Marshal.UnsafeAddrOfPinnedArrayElement(buffer2, segments[i].Offset) = buffer1[count++];
            }
            segments.RemoveRange(0, i);
        }

        foreach (GCHandle handle in buffer1Handles) handle.Free();
        foreach (GCHandle handle in buffer2Handles) handle.Free();

        Assert.Equal(string.Join(",", buffer1), string.Join(",", buffer2));
    }

results in:

MyTests.Test(copyLength: 1024) [FAIL]
        Assert.Equal() Failure
                                         ↓ (pos 3658)
        Expected: ···1,252,253,254,255,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17···
        Actual:   ···1,252,253,254,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0···
                                         ↑ (pos 3658)

Presumably the changes from the PR come into play because of that RemoveRange call.

cc: @jkotas

@stephentoub
Copy link
Member Author

(Looks like it made it past coreclr CI because the System.Net.Sockets tests are disabled in coreclr CI: https://github.com/dotnet/coreclr/blob/e5add1d2e4a3a62c0ced24f6b63b9c72e1206f24/tests/CoreFX/CoreFX.issues.rsp#L46)

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant