Increment by 'Actual bytes read' rather than the buffer size in StreamsComparer.cs

[mithileshz]

Fixes #4684

This PR makes further changes as discussed in the comments for PR #4668.

While thinking through the change discussed, I realised that we don't need to have line 85 anymore as the byte arrays have a difference and therefore the for loop will return and not continue this loop.

I'm not 100% sure how to create a test to cover this scenario but all the existing tests pass.

[stevenaw]

Great find on this not being needed!

One thing you can try for a test is to have the tests define a new class which extends MemoryStream and overrides Read(byte[] buffer, int offset, int count) method so that it calls base.Read(buffer, offset, 2) to simulate a steam which always returns less than the asked for amount. Your test can then instantiate two copies of this class, one for expected and one for actual, each of which can be given a small byte array of equal length but with a differing value somewhere near the end of the array.

private class ShortReadingMemoryStream : MemoryStream
{
    public ShortReadingMemoryStream(byte[] bytes) : base(bytes) { }

    public override int Read(byte[] buffer, int offset, int count)
    {
        return base.Read(buffer, offset, 2);
    }
}

We may also want to change line 71 above to end use readActual instead of BUFFER_SIZE in the for loop declaration to avoid reading past the part of the buffer filled by the Read().
EDIT: I've removed this second suggestion

[mithileshz]

So from my testing this is what I have figured out:

• The readByte is only being used to set the Position of the FailurePoint in the NUnitEqualityComparer. It's not used for actual reading of the object.
• When we override the Read method and return only 2 bytes instead of the BUFFER_SIZE the readExpected and readActual would still return true in the while condition. So the while loop is actually doing 1 extra loop than it needs to as the readExpected and readActual would be the result of the previous loops value.

I tried turning it into a do while loop (code below) but that seems to break the logic in the scenario that we're testing with where the .Read method has been overridden with a custom value rather than the BUFFER_SIZE

I've added a test to fix this scenario now. Let me know if it's all good :)

do
{
    readExpected = binaryReaderExpected.Read(bufferExpected, 0, BUFFER_SIZE);
    readActual = binaryReaderActual.Read(bufferActual, 0, BUFFER_SIZE);

    if (MemoryExtensions.SequenceEqual<byte>(bufferExpected, bufferActual))
    {
        readByte += readActual;
        continue;
    }

    for (int count = 0; count < BUFFER_SIZE; ++count)
    {
        if (bufferExpected[count] != bufferActual[count])
        {
            NUnitEqualityComparer.FailurePoint fp = new NUnitEqualityComparer.FailurePoint();
            fp.Position = readByte + count;
            fp.ExpectedHasData = true;
            fp.ExpectedValue = bufferExpected[count];
            fp.ActualHasData = true;
            fp.ActualValue = bufferActual[count];
            equalityComparer.FailurePoints.Insert(0, fp);
            return EqualMethodResult.ComparedNotEqual;
        }
    }
}
while (readExpected == BUFFER_SIZE && readActual == BUFFER_SIZE);

[mithileshz]

Actually, while doing more investigation around this change, Linq has a SequenceEquals which gives the same performance enhancement as my previous PR whilst removing one of the dependancies I added in the previous PR (System.Memory).

I've also pushed the changes in this PR as I think this might be better? Let me know if you'd like me to separate the PRs or if MemoryExtensions is the way to go?

|     Method |  Size |      Mean |     Error |    StdDev | Ratio |
|----------- |------ |----------:|----------:|----------:|------:|
|   Original | 32768 | 17.890 us | 0.0576 us | 0.0481 us |  1.00 |
|       Linq | 32768 |  2.099 us | 0.0415 us | 0.0494 us |  0.12 |
| Vectorized | 32768 |  2.029 us | 0.0222 us | 0.0197 us |  0.11 |

[stevenaw]

Nice investigating and good find about the System.Linq option to maybe avoid the dependency!
I recall you saying before that you were having a hard time running benchmarks on .NET Framework, are you able to confirm if you're seeing the same improvement on the .NET Framework side? I'm not sure, but I suspect you may be hitting some .NET 8 optimizations there that may not be present on the .NET Framework side. It would at least be something we'd want to confirm/rule out :)

[OsirisTerje]

@mithileshz @stevenaw Is this ready now to be merged? Any more to be done here?

[stevenaw]

Thanks for following up for me @OsirisTerje .
@nunit/framework-team A few things have come up and I'll need to ask for some help reviewing this one in a timely manner. Would anyone be able to take over helping @mithileshz getting these changes merged?

[OsirisTerje]

@mithileshz

I recall you saying before that you were having a hard time running benchmarks on .NET Framework, are you able to confirm if you're seeing the same improvement on the .NET Framework side?

Is it working for .net fx ?
Is there anything else you need to do here?

[mithileshz]

Hey @OsirisTerje
Sorry I've been busy last few weeks. Here's the benchmarks with the new changes in .NET Framework:

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22621.3296/22H2/2022Update/SunValley2)
13th Gen Intel Core i5-13420H, 1 CPU, 12 logical and 8 physical cores
  [Host]     : .NET Framework 4.8.1 (4.8.9181.0), X86 LegacyJIT
  DefaultJob : .NET Framework 4.8.1 (4.8.9181.0), X86 LegacyJIT


| Method     | Size  | Mean       | Error     | StdDev    | Ratio | RatioSD |
|----------- |------ |-----------:|----------:|----------:|------:|--------:|
| Original   | 32768 |  18.093 us | 0.0502 us | 0.0470 us |  1.00 |    0.00 |
| Linq       | 32768 | 234.799 us | 1.9936 us | 1.8648 us | 12.98 |    0.10 |
| Vectorized | 32768 |   5.535 us | 0.0264 us | 0.0247 us |  0.31 |    0.00 |

And these were the benchmarks in .net core:

|     Method |  Size |      Mean |     Error |    StdDev | Ratio |
|----------- |------ |----------:|----------:|----------:|------:|
|   Original | 32768 | 17.890 us | 0.0576 us | 0.0481 us |  1.00 |
|       Linq | 32768 |  2.099 us | 0.0415 us | 0.0494 us |  0.12 |
| Vectorized | 32768 |  2.029 us | 0.0222 us | 0.0197 us |  0.11 |

So the conclusion is that in .NET Core, the Linq method was optimised but in .NET Framework it's much worse. This means that I need to revert the this commit and keep the original changes. I'll do so now and this PR should then be ready to merge!

Edit: It's ready to be reviewed and merged!

[OsirisTerje]

@mithileshz Awesome! And thanks for checking that out !

[stevenaw]

This looks great to me, thanks for the contribution @mithileshz ! Thanks for checking the .NET Framework side of this too