Runtime: System.IO.Pipelines.ReadResult should be readonly

[benaadams]

So it can be efficiently passed with in

public readonly struct ReadResult
{
    internal readonly ReadOnlySequence<byte> _resultBuffer;
    internal readonly ResultFlags _resultFlags;
}

/cc @davidfowl @pakrym

[pakrym]

Wow, how did we miss it.

[benaadams]

Luckily changing it isn't breaking change :)

[CodeTherapist]

Maybe I misunderstood the task.
The proposed change would not work with the current code in the master branch because the pipe sets the fields at Pipe.cs (796), Pipe.cs (802) and Pipe.cs (811).

[benaadams]

They'd need to change

[CodeTherapist]

@benaadams Thanks for the information.
I'm still interested doing this task - let me do another PR for it, with the additional changes to Pipe.cs.

[benaadams]

private void GetReadResult(ref ReadResult result)

Should probably be

private ReadResult GetReadResult()

[pakrym]

That was done because of performance reasons (to avoid copying struct and decrease stack size). So it would be interesting to see perf impact.

[benaadams]

Ah cos ReadOnlySequence<byte> is chunky...

[benaadams]

private void GetReadResult(out ReadResult result) would have similar effect to ref?

[CodeTherapist]

The out keyword causes arguments to be passed by reference.

@pakrym I'm new to this corefx development - how could I check the perf impact of my change?

[pakrym]

cc @ahsonkhan can you help here?

The most interesting scenario is repeatedly reading from the pipe without advancing all the way so the sync code path is used all the time.

[ahsonkhan]

can you help here?

Sure, I can try :)

how could I check the perf impact of my change?

If you have a performance test that captures the scenario you are interested in, you can run it by following the steps here:
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/performance-tests.md

You would have to write the relevant performance test first (if one doesn't exist). Let me know if you have any questions.

[CodeTherapist]

Thanks to everyone that is helping me on that.

@pakrym
Now I did a performance test accordingly to the documentation and it looks like that there is no performance impact by this change. Please let me know what do you think about the perf test. When the perf test is good, I would add it to the PR.

With my changes (Branch)

System.IO.Pipelines.Performance.Tests.dll | Metric | Unit | Iterations | Average | STDEV.S | Min | Max
:--------------------------------------------- |:-------- |:----:|:----------:| -------:| -------:| -----:| -----:
System.IO.Pipelines.Tests.Perf_Pipe.ReadAsync | Duration | msec | 1000 | 0.015 | 0.017 | 0.009 | 0.213

Without my changes (Branch)

System.IO.Pipelines.Performance.Tests.dll | Metric | Unit | Iterations | Average | STDEV.S | Min | Max
:--------------------------------------------- |:-------- |:----:|:----------:| -------:| -------:| -----:| -----:
System.IO.Pipelines.Tests.Perf_Pipe.ReadAsync | Duration | msec | 1000 | 0.015 | 0.018 | 0.009 | 0.268

[ahsonkhan]

@CodeTherapist, you need to add an inneriterationcount (attribute on the benchmark and a a loop). The tests should take 1+ ms. Otherwise, the results are too noisy to see a difference.

[CodeTherapist]

@ahsonkhan Thanks. Is that correct?

With my changes (Branch)

System.IO.Pipelines.Performance.Tests.dll | Metric | Unit | Iterations | Average | STDEV.S | Min | Max|
--------------------------------------------- |:-------- |:----:|:----------:| --------:| -------:| --------:| --------|
System.IO.Pipelines.Tests.Perf_Pipe.ReadAsync | Duration | msec | 4 | 2510.920 | 7.606 | 2503.556 | 2517.946|

Without my changes (Branch)

System.IO.Pipelines.Performance.Tests.dll | Metric | Unit | Iterations | Average | STDEV.S | Min | Max
:--------------------------------------------- |:-------- |:----:|:----------:| --------:| -------:| --------:| --------:
System.IO.Pipelines.Tests.Perf_Pipe.ReadAsync | Duration | msec | 4 | 2584.346 | 12.623 | 2572.661 | 2602.189

[pakrym]

Good job!

There is a slight adjustment you can do to isolate scenario even more:

Move

await writer.WriteAsync(new byte[] { 0, 0, 0, 0 });
await writer.FlushAsync();

outside the loop and call reader.AdvanceTo(result.Buffer.Start);. This way you are going to eliminate any writing overhead and just exercise sync reading.

[CodeTherapist]

@pakrym Thanks for the hint - I did the changes.

With my changes (Branch)

System.IO.Pipelines.Performance.Tests.dll | Metric | Unit | Iterations | Average | STDEV.S | Min | Max
:--------------------------------------------- |:-------- |:----:|:----------:| -------:| -------:| -----:| -----:
System.IO.Pipelines.Tests.Perf_Pipe.ReadAsync | Duration | msec | 100 | 1.241 | 0.009 | 1.235 | 1.295

Without my changes (Branch)

System.IO.Pipelines.Performance.Tests.dll | Metric | Unit | Iterations | Average | STDEV.S | Min | Max
:--------------------------------------------- |:-------- |:----:|:----------:| -------:| -------:| -----:| -----:
System.IO.Pipelines.Tests.Perf_Pipe.ReadAsync | Duration | msec | 100 | 1.179 | 0.009 | 1.164 | 1.216