Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Change BulkMoveWithWriteBarrier to be GC suspension friendly #27776

Merged
merged 3 commits into from
Nov 9, 2019

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Nov 8, 2019

Micro-benchmark results:

// Mean pause time: 
// Baseline: 32.9ms
// This change: 21ms
static void Main()
{
    new Thread(() =>
    {
        var src = new object[10_000_000]; 
        var dst = new object[10_000_000]; 
        for (;;) Array.Copy(src, dst, src.Length);
    }).Start();
    for (;;) { GC.Collect(); Thread.Sleep(1); }
}
// Mean pause time: 
// Baseline: 33.1ms
// This change: 0.8ms
static void Main()
{
    new Thread(() =>
    {
        var a = new byte[100_000_000];
        for (;;) a.Clone();
    }).Start();
    for (;;) { GC.Collect(); Thread.Sleep(1); }
}

Fixes #27769

@jkotas jkotas requested a review from VSadov November 8, 2019 19:15
@danmoseley
Copy link
Member

Do we have missing coverage in more specific corefx tests than networking? eg Array.Copy?

@jkotas
Copy link
Member Author

jkotas commented Nov 8, 2019

The problem would be only hit for a very specific input: the size of the block must be more than 16kB, the array being copied must contain GC references, the destination must be overlap with source in specific way.

I have changed the threshold for debug builds to be 1kB instead of 16kB that makes it much more likely for bugs like this to be caught. The original bug would by caught by more tests in CoreFX with the debug-build threshold.

if (Unsafe.AreSame(ref source, ref destination))
return;

if ((nuint)(nint)Unsafe.ByteOffset(ref source, ref destination) >= byteCount)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed - ByteOffset(a, b) is b - a

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy to miss when reading late in the day.
Maybe add a comment that this is folded (destination - source) >= byteCount || (destination - source) < 0 - just for the next one who reads this.

@VSadov
Copy link
Member

VSadov commented Nov 8, 2019

@stephentoub - about the test. Is that normal for sockets or the test was stressing some limits?

@stephentoub
Copy link
Member

stephentoub commented Nov 8, 2019

about the test. Is that normal for sockets or the test was stressing some limits?

The test was written to validate behavior when more segments are passed into a Receive/Send call than IOV_MAX, the maximum the number the platform can handle. In that case we need to split the segments into multiple calls. The test is validating that all data is sent/received appropriately when we need to do such handling.

I definitely don't expect this number of segments to be used in any real workload. Generally the number is quite small (e.g. 2), not 2400 :)

@stephentoub stephentoub merged commit fc1e6ce into dotnet:master Nov 9, 2019
@jkotas jkotas deleted the issue-27769 branch November 10, 2019 01:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change BulkMoveWithWriteBarrier to be GC suspension friendly
5 participants