Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change if order to improve FileStream perf for cases when both buffers are of the same size #51489

Merged
merged 3 commits into from
May 10, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -353,18 +353,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;
}
Expand Down Expand Up @@ -649,19 +650,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:
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
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);
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
}
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;
}
Expand Down