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

Use System.Buffers in System.IO.FileSystem #5954

Merged
merged 1 commit into from
Feb 9, 2016

Conversation

stephentoub
Copy link
Member

  • Change FileStream to use ArrayPool<byte>.Shared for its internal buffer
  • Add to Common a helper implementation of CopyToAsync that uses ArrayPool<byte>.Shared until such type as the base Stream (in mscorlib) can use a pooled buffer
  • Utilize that CopyToAsync helper in FileStream

Having added that:

  • Utilize that CopyToAsync helper in DeflateStream/GZipStream, as System.IO.Compression already depends on System.Buffers.

cc: @ericstij, @ianhays, @sokket, @KrzysztofCwalina
https://github.com/dotnet/corefx/issues/5598

while ((bytesRead = await source.ReadAsync(buffer, 0, buffer.Length, cancellationToken).ConfigureAwait(false)) != 0)
{
await destination.WriteAsync(buffer, 0, bytesRead, cancellationToken).ConfigureAwait(false);
}
Copy link
Member

Choose a reason for hiding this comment

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

Break read+write dependency? e.g.

    byte[] buffer0 = ArrayPool<byte>.Shared.Rent(bufferSize);
    byte[] buffer1 = ArrayPool<byte>.Shared.Rent(bufferSize);
    return ArrayPoolCopyToAsyncInternal(source, destination, buffer0, buffer1, cancellationToken);
}

private static async Task ArrayPoolCopyToAsyncInternal(Stream source, Stream destination, byte[] buffer0, byte[] buffer1, CancellationToken cancellationToken)
{
    try
    {
        int bytesRead;
        Task<int> lastReadTask = source.ReadAsync(buffer0, 0, buffer0.Length, cancellationToken);
        Task lastWriteTask = Task.CompletedTask;
        bool otherBuffer = true;

        while ((bytesRead = await lastReadTask.ConfigureAwait(false)) != 0)
        {
            lastReadTask = source.ReadAsync((otherBuffer ? buffer1 : buffer0), 0, (otherBuffer ? buffer1 : buffer0).Length, cancellationToken);
            await lastWriteTask.ConfigureAwait(false);
            lastWriteTask = destination.WriteAsync((otherBuffer ? buffer0 : buffer1), 0, bytesRead, cancellationToken);
            otherBuffer = !otherBuffer;
        }
        await lastWriteTask.ConfigureAwait(false);
    }
    finally
    {
        ArrayPool<byte>.Shared.Return(buffer0);
        ArrayPool<byte>.Shared.Return(buffer1);
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that but decided against it for the same reasons we didn't do so in the original Stream.CopyToAsync implementation. It leads to interesting concurrency issues that we decided to avoid in these core methods. For example, if the two concurrent read and write operations throw, which exception should we propagate, and what does that mean if there were important information in the other? And if the operations would actually end up consuming a thread pool thread to do a synchronous operation, do we really want to potentially be blocking two thread pool threads rather than one? Etc. If someone determines that they would really benefit from such overlapping, they can make that decision at the app level (where they have the ability to measure impact at the app-level) and write the few lines like you have themselves. Thanks, though.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense

@stephentoub stephentoub force-pushed the fs_buffers branch 2 times, most recently from 7a7ec87 to 0a4bfb5 Compare February 8, 2016 03:39
@ericstj
Copy link
Member

ericstj commented Feb 8, 2016

LGTM

@@ -107,7 +107,7 @@ internal void InitializeDeflater(Stream stream, bool leaveOpen, int windowBits,
{
Debug.Assert(stream != null);
if (!stream.CanWrite)
throw new ArgumentException(SR.NotWriteableStream, "stream");
throw new ArgumentException(SR.NotSupported_UnreadableStream, "stream");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be NotSupported_UnwritableStream?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it should. Will fix.

@jonmill
Copy link

jonmill commented Feb 8, 2016

LGTM!

The default CopyToAsync on Stream uses an 80K temporary buffer, making it a good candidate for buffer pooling.  But Stream is in mscorlib and can't rely on System.Buffers.  So this commit introduces an implementation that streams in mscorlib can use, just by overriding CopyToAsync and delegating.  It then uses that in DeflateStream, GZipStream, FileStream, PipeStream, and UnmanagedMemoryStream.
stephentoub added a commit that referenced this pull request Feb 9, 2016
Use System.Buffers in System.IO.FileSystem
@stephentoub stephentoub merged commit f6cdaf2 into dotnet:master Feb 9, 2016
@stephentoub stephentoub deleted the fs_buffers branch February 9, 2016 17:23
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Use System.Buffers in System.IO.FileSystem

Commit migrated from dotnet/corefx@f6cdaf2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants