Change BulkMoveWithWriteBarrier to be GC suspension friendly by jkotas · Pull Request #27776 · dotnet/coreclr (original) (raw)

This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation6 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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

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.

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

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.

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

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