Use ValueTask-returning async FileStream methods wherever possible

[adamsitnik]

With #50802 and the next upcoming PR from @carlossanlop the following FileStream methods are going to become allocation-free:

ValueTask<int> ReadAsync(Memory<byte> destination, CancellationToken cancellationToken = default)
ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default)

The "old", Task-returning overloads are still going to be allocating memory:

Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)

The person who is willing to work on this task should:

1. Search the entire runtime repo for places where the mentioned Task-returning methods are used. Please keep in mind that it's possible that we are already doing the right thing and you won't find any places for improvements.
2. If you find places that can be improved, contribute benchmarks to the dotnet/performance repo after reading the Microbenchmark Design Guidelines doc. The benchmarks need to cover the code that is about to be changed.
3. Run the benchmarks following the Benchmarking workflow for dotnet/runtime repository doc and ensure the changes are improving perf (with a focus on the managed allocations).
4. Contribute the improvements to dotnet/runtime and provide benchmark results

Note: some of the libraries are still targeting .NET Standard or even Full .NET Framework. For these libs, you might need to use conditional compilation like in the example below:

runtime/src/libraries/System.IO.FileSystem/src/System/IO/File.cs

Lines 986 to 990 in 0af5228

	#if MS_IO_REDIST
	await sw.WriteAsync(buffer, 0, batchSize).ConfigureAwait(false);
	#else
	await sw.WriteAsync(new ReadOnlyMemory<char>(buffer, 0, batchSize), cancellationToken).ConfigureAwait(false);
	#endif

[ghost]

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

---

With #50802 and the next upcoming PR from @carlossanlop the following FileStream methods are going to become allocation-free:

ValueTask<int> ReadAsync(Memory<byte> destination, CancellationToken cancellationToken = default)
ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default)

The "old", Task-returning overloads are still going to be allocating memory:

Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)

The person who is willing to work on this task should:

1. Search the entire runtime repo for places where the mentioned Task-returning methods are used. Please keep in mind that it's possible that we are already doing the right thing and you won't find any places for improvements.
2. If you find places that can be improved, contribute benchmarks to the dotnet/performance repo after reading the Microbenchmark Design Guidelines doc. The benchmarks need to cover the code that is about to be changed.
3. Run the benchmarks following the Benchmarking workflow for dotnet/runtime repository doc and ensure the changes are improving perf (with a focus on the managed allocations).
4. Contribute the improvements to dotnet/runtime and provide benchmark results

Note: some of the libraries are still targeting .NET Standard or even Full .NET Framework. For these libs, you might need to use conditional compilation like in the example below:

runtime/src/libraries/System.IO.FileSystem/src/System/IO/File.cs

Lines 986 to 990 in 0af5228

	#if MS_IO_REDIST
	await sw.WriteAsync(buffer, 0, batchSize).ConfigureAwait(false);
	#else
	await sw.WriteAsync(new ReadOnlyMemory<char>(buffer, 0, batchSize), cancellationToken).ConfigureAwait(false);
	#endif

Author:	adamsitnik
Assignees:	-
Labels:	area-System.IO, tenet-performance, up-for-grabs
Milestone:	6.0.0

[stephentoub]

Search the entire runtime repo for places where the mentioned Task-returning methods are used. Please keep in mind that it's possible that we are already doing the right thing and you won't find any places for improvements.

This is unlikely to be very fruitful. I've previously made passes through to fix all such stream usage, and we now have an analyzer that flags such usage.

runtime/eng/CodeAnalysis.ruleset

Line 115 in 0af5228

<Rule Id="CA1835" Action="Warning" /> <!-- Prefer the 'Memory'-based overloads for 'ReadAsync' and 'WriteAsync' -->

[adamsitnik]

This is unlikely to be very fruitful.

@stephentoub I agree, I just want someone to set it to Error and ensure that we are not doing it anywhere.

we now have an analyzer that flags such usage.

Thanks! We are going to mention the analyser in our FileStream perf best practices docs 👍

[stephentoub]

I just want someone to set it to Error

What does this mean? Do you mean the analyzer? We treat warnings as errors.

[adamsitnik]

We treat warnings as errors.

I was not aware of that. In such a case, I am just going to close the issue. Thanks!