From de4c4815b26014e4b78761d2b7e7ad6e4eea6c6d Mon Sep 17 00:00:00 2001
From: Adam Sitnik <adam.sitnik@gmail.com>
Date: Mon, 10 May 2021 23:32:58 +0200
Subject: [PATCH] change if order to improve FileStream perf for cases when
 both buffers are of the same size (#51489)

* change if order to improve the perf for case where buffer.Length == _bufferSize

* add comment that explains what we are doing and why

* apply the optimization to ReadAsync as well
---
 .../Strategies/BufferedFileStreamStrategy.cs  | 39 ++++++++++++-------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs
index c39d2ae49b3c6..2228993f48e5a 100644
--- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs
@@ -354,18 +354,19 @@ public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken
                 bool releaseTheLock = true;
                 try
                 {
-                    if (_readLen - _readPos >= buffer.Length)
+                    if (_readLen == _readPos && buffer.Length >= _bufferSize)
                     {
-                        // hot path #1: there is enough data in the buffer
+                        // hot path #1: the read buffer is empty and buffering would not be beneficial
+                        // To find out why we are bypassing cache here, please see WriteAsync comments.
+                        return _strategy.ReadAsync(buffer, cancellationToken);
+                    }
+                    else if (_readLen - _readPos >= buffer.Length)
+                    {
+                        // hot path #2: there is enough data in the buffer
                         _buffer.AsSpan(_readPos, buffer.Length).CopyTo(buffer.Span);
                         _readPos += buffer.Length;
                         return new ValueTask<int>(buffer.Length);
                     }
-                    else if (_readLen == _readPos && buffer.Length >= _bufferSize)
-                    {
-                        // hot path #2: the read buffer is empty and buffering would not be beneficial
-                        return _strategy.ReadAsync(buffer, cancellationToken);
-                    }
 
                     releaseTheLock = false;
                 }
@@ -650,19 +651,29 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationTo
                 bool releaseTheLock = true;
                 try
                 {
-                    // hot path #1 if the write completely fits into the buffer, we can complete synchronously:
-                    if (_bufferSize - _writePos >= buffer.Length)
+                    // hot path #1: the write buffer is empty and buffering would not be beneficial
+                    if (_writePos == 0 && buffer.Length >= _bufferSize)
                     {
+                        // The fact that Strategy can be wrapped by BufferedFileStreamStrategy
+                        // is transparent to every Strategy implementation. It means, that
+                        // every Strategy must work fine no matter if buffering is enabled or not.
+                        // In case of AsyncWindowsFileStreamStrategy.WriteAsync,
+                        // it updates it's private position BEFORE it enqueues the IO request.
+                        // This combined with the fact that BufferedFileStreamStrategy state
+                        // is not modified here, allows us to NOT await the call
+                        // and release the lock BEFORE the IO request completes.
+                        // It improves the performance of common scenario, where buffering is enabled (default)
+                        // but the user provides buffers larger (or equal) to the internal buffer size.
+                        return _strategy.WriteAsync(buffer, cancellationToken);
+                    }
+                    else if (_bufferSize - _writePos >= buffer.Length)
+                    {
+                        // hot path #2 if the write completely fits into the buffer, we can complete synchronously:
                         EnsureBufferAllocated();
                         buffer.Span.CopyTo(_buffer.AsSpan(_writePos));
                         _writePos += buffer.Length;
                         return default;
                     }
-                    else if (_writePos == 0 && buffer.Length >= _bufferSize)
-                    {
-                        // hot path #2: the write buffer is empty and buffering would not be beneficial
-                        return _strategy.WriteAsync(buffer, cancellationToken);
-                    }
 
                     releaseTheLock = false;
                 }