Added a factory to FileStreamCompletionSource (#16190)

* Merged FileStreamCompletionSource and MemoryCompletionSource
- Change FileStreamCompletionSource to take a ReadOnlyMemory<byte> instead of byte[]
- Delegate to memory.Retain if the underlying ReadOnlyMemory<byte> is backed by an array that isn't the stream's internal buffer.

* Brought back MemoryFileStreamCompletionSource
- Added a factory method to FileStreamCompletionSource that
picks the an impl or FileStreamCompletionSource or MemoryFileStreamCompletionSource based on the buffer matching the underlying stream's internal buffer.

* PR feedback
- Cleanup the condition
- Flip to Debug.Assert in FileStreamCompletionSource

* Check for null before using the PreAllocatedOverlapped

diff --git a/src/mscorlib/shared/System/IO/FileStream.Windows.cs b/src/mscorlib/shared/System/IO/FileStream.Windows.cs
index 477b943..fa9c0de 100644 (file)
--- a/src/mscorlib/shared/System/IO/FileStream.Windows.cs
+++ b/src/mscorlib/shared/System/IO/FileStream.Windows.cs
@@ -848,9 +848,7 @@ namespace System.IO
             Debug.Assert(_useAsyncIO, "ReadNativeAsync doesn't work on synchronous file streams!");
 
             // Create and store async stream class library specific data in the async result
-            FileStreamCompletionSource completionSource = destination.TryGetArray(out ArraySegment<byte> memoryArray) ?
-                new FileStreamCompletionSource(this, numBufferedBytesRead, memoryArray.Array) :
-                new MemoryFileStreamCompletionSource(this, numBufferedBytesRead, destination);
+            FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, numBufferedBytesRead, destination);
             NativeOverlapped* intOverlapped = completionSource.Overlapped;
 
             // Calculate position in the file we should be at after the read is done
@@ -1069,9 +1067,7 @@ namespace System.IO
             Debug.Assert(_useAsyncIO, "WriteInternalCoreAsync doesn't work on synchronous file streams!");
 
             // Create and store async stream class library specific data in the async result
-            FileStreamCompletionSource completionSource = MemoryMarshal.TryGetArray(source, out ArraySegment<byte> array) ?
-                new FileStreamCompletionSource(this, 0, array.Array) :
-                new MemoryFileStreamCompletionSource(this, 0, source);
+            FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, 0, source);
             NativeOverlapped* intOverlapped = completionSource.Overlapped;
 
             if (CanSeek)

diff --git a/src/mscorlib/shared/System/IO/FileStreamCompletionSource.Win32.cs b/src/mscorlib/shared/System/IO/FileStreamCompletionSource.Win32.cs
index 4e19f46..82274b1 100644 (file)
--- a/src/mscorlib/shared/System/IO/FileStreamCompletionSource.Win32.cs
+++ b/src/mscorlib/shared/System/IO/FileStreamCompletionSource.Win32.cs
@@ -36,7 +36,7 @@ namespace System.IO
             private long _result; // Using long since this needs to be used in Interlocked APIs
 
             // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations)
-            internal FileStreamCompletionSource(FileStream stream, int numBufferedBytes, byte[] bytes)
+            protected FileStreamCompletionSource(FileStream stream, int numBufferedBytes, byte[] bytes)
                 : base(TaskCreationOptions.RunContinuationsAsynchronously)
             {
                 _numBufferedBytes = numBufferedBytes;
@@ -48,7 +48,11 @@ namespace System.IO
                 // thus is already pinned) and if no one else is currently using the preallocated overlapped.  This is the fast-path
                 // for cases where the user-provided buffer is smaller than the FileStream's buffer (such that the FileStream's
                 // buffer is used) and where operations on the FileStream are not being performed concurrently.
-                _overlapped = (bytes == null || ReferenceEquals(bytes, _stream._buffer)) && _stream.CompareExchangeCurrentOverlappedOwner(this, null) == null ?
+                Debug.Assert((bytes == null || ReferenceEquals(bytes, _stream._buffer)));
+
+                // The _preallocatedOverlapped is null if the internal buffer was never created, so we check for 
+                // a non-null bytes before using the stream's _preallocatedOverlapped
+                _overlapped = bytes != null && _stream.CompareExchangeCurrentOverlappedOwner(this, null) == null ?
                     _stream._fileHandle.ThreadPoolBinding.AllocateNativeOverlapped(_stream._preallocatedOverlapped) :
                     _stream._fileHandle.ThreadPoolBinding.AllocateNativeOverlapped(s_ioCallback, this, bytes);
                 Debug.Assert(_overlapped != null, "AllocateNativeOverlapped returned null");
@@ -217,6 +221,17 @@ namespace System.IO
                     }
                 }
             }
+
+            public static FileStreamCompletionSource Create(FileStream stream, int numBufferedBytesRead, ReadOnlyMemory<byte> memory)
+            {
+                // If the memory passed in is the stream's internal buffer, we can use the base FileStreamCompletionSource,
+                // which has a PreAllocatedOverlapped with the memory already pinned.  Otherwise, we use the derived
+                // MemoryFileStreamCompletionSource, which Retains the memory, which will result in less pinning in the case
+                // where the underlying memory is backed by pre-pinned buffers.
+                return MemoryMarshal.TryGetArray(memory, out ArraySegment<byte> buffer) && ReferenceEquals(buffer.Array, stream._buffer) ?
+                    new FileStreamCompletionSource(stream, numBufferedBytesRead, buffer.Array) :
+                    new MemoryFileStreamCompletionSource(stream, numBufferedBytesRead, memory);
+            }
         }
 
         /// <summary>
@@ -231,7 +246,6 @@ namespace System.IO
             internal MemoryFileStreamCompletionSource(FileStream stream, int numBufferedBytes, ReadOnlyMemory<byte> memory) :
                 base(stream, numBufferedBytes, bytes: null) // this type handles the pinning, so null is passed for bytes
             {
-                Debug.Assert(!MemoryMarshal.TryGetArray(memory, out ArraySegment<byte> array), "The base should be used directly if we can get the array.");
                 _handle = memory.Retain(pin: true);
             }