From 91423fa3d672ed622c758b57f570b7a25bc12b33 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 4 Mar 2021 10:22:05 +0100 Subject: [PATCH] track the file offset in memory, don't use expensive sys calls to synchronize it --- ...op.ReadFile_SafeHandle_NativeOverlapped.cs | 8 +++ ...p.WriteFile_SafeHandle_NativeOverlapped.cs | 3 ++ .../IO/AsyncWindowsFileStreamStrategy.cs | 44 ++++++++-------- .../System/IO/FileStreamHelpers.Windows.cs | 36 ++++++++++--- .../IO/LegacyFileStreamStrategy.Windows.cs | 4 +- .../IO/SyncWindowsFileStreamStrategy.cs | 16 +++++- .../System/IO/WindowsFileStreamStrategy.cs | 50 +++++++++++-------- 7 files changed, 106 insertions(+), 55 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs index 8419932249299..c6fb2c4311cf3 100644 --- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs +++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs @@ -16,5 +16,13 @@ internal static extern unsafe int ReadFile( int numBytesToRead, IntPtr numBytesRead_mustBeZero, NativeOverlapped* overlapped); + + [DllImport(Libraries.Kernel32, SetLastError = true)] + internal static extern unsafe int ReadFile( + SafeHandle handle, + byte* bytes, + int numBytesToRead, + out int numBytesRead, + NativeOverlapped* overlapped); } } diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs index cca93e0b3ac89..3eaff9ca07626 100644 --- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs +++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs @@ -17,5 +17,8 @@ internal static partial class Kernel32 // and pass in an address for the numBytesRead parameter. [DllImport(Libraries.Kernel32, SetLastError = true)] internal static extern unsafe int WriteFile(SafeHandle handle, byte* bytes, int numBytesToWrite, IntPtr numBytesWritten_mustBeZero, NativeOverlapped* lpOverlapped); + + [DllImport(Libraries.Kernel32, SetLastError = true)] + internal static extern unsafe int WriteFile(SafeHandle handle, byte* bytes, int numBytesToWrite, out int numBytesWritten, NativeOverlapped* lpOverlapped); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs index 7cde9a24730ff..abc30c94c33fd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs @@ -140,15 +140,16 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio NativeOverlapped* intOverlapped = completionSource.Overlapped; // Calculate position in the file we should be at after the read is done + long positionBefore = _filePosition; if (CanSeek) { long len = Length; - if (_filePosition + destination.Length > len) + if (positionBefore + destination.Length > len) { - if (_filePosition <= len) + if (positionBefore <= len) { - destination = destination.Slice(0, (int)(len - _filePosition)); + destination = destination.Slice(0, (int)(len - positionBefore)); } else { @@ -158,23 +159,17 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. - intOverlapped->OffsetLow = unchecked((int)_filePosition); - intOverlapped->OffsetHigh = (int)(_filePosition >> 32); + intOverlapped->OffsetLow = unchecked((int)positionBefore); + intOverlapped->OffsetHigh = (int)(positionBefore >> 32); // When using overlapped IO, the OS is not supposed to // touch the file pointer location at all. We will adjust it - // ourselves. This isn't threadsafe. - - // WriteFile should not update the file pointer when writing - // in overlapped mode, according to MSDN. But it does update - // the file pointer when writing to a UNC path! - // So changed the code below to seek to an absolute - // location, not a relative one. ReadFile seems consistent though. - SeekCore(_fileHandle, destination.Length, SeekOrigin.Current); + // ourselves, but only in memory. This isn't threadsafe. + _filePosition += destination.Length; } // queue an async ReadFile operation and pass in a packed overlapped - int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, intOverlapped, out int errorCode); + int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, false, intOverlapped, out int errorCode); // ReadFile, the OS version, will return 0 on failure. But // my ReadFileNative wrapper returns -1. My wrapper will return @@ -203,7 +198,7 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio { if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. { - SeekCore(_fileHandle, 0, SeekOrigin.Current); + _filePosition = positionBefore; } completionSource.ReleaseNativeResource(); @@ -264,29 +259,30 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, _preallocatedOverlapped, 0, source); NativeOverlapped* intOverlapped = completionSource.Overlapped; + long positionBefore = _filePosition; if (CanSeek) { // Make sure we set the length of the file appropriately. long len = Length; - if (_filePosition + source.Length > len) + if (positionBefore + source.Length > len) { - SetLengthCore(_filePosition + source.Length); + SetLengthCore(positionBefore + source.Length); } // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. - intOverlapped->OffsetLow = (int)_filePosition; - intOverlapped->OffsetHigh = (int)(_filePosition >> 32); + intOverlapped->OffsetLow = (int)positionBefore; + intOverlapped->OffsetHigh = (int)(positionBefore >> 32); // When using overlapped IO, the OS is not supposed to // touch the file pointer location at all. We will adjust it - // ourselves. This isn't threadsafe. - SeekCore(_fileHandle, source.Length, SeekOrigin.Current); + // ourselves, but only in memory. This isn't threadsafe. + _filePosition += source.Length; } // queue an async WriteFile operation and pass in a packed overlapped - int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, intOverlapped, out int errorCode); + int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, false, intOverlapped, out int errorCode); // WriteFile, the OS version, will return 0 on failure. But // my WriteFileNative wrapper returns -1. My wrapper will return @@ -312,7 +308,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella { if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. { - SeekCore(_fileHandle, 0, SeekOrigin.Current); + _filePosition = positionBefore; } completionSource.ReleaseNativeResource(); @@ -385,7 +381,7 @@ private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, Canc // Make sure the stream's current position reflects where we ended up if (!_fileHandle.IsClosed && CanSeek) { - SeekCore(_fileHandle, 0, SeekOrigin.End); + _filePosition = Length; } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs index 840d62cb03b80..d1d2cdb42686a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs @@ -337,7 +337,7 @@ internal static unsafe void SetLength(SafeFileHandle handle, string? path, long } // __ConsoleStream also uses this code. - internal static unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, NativeOverlapped* overlapped, out int errorCode) + internal static unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, bool syncUsingOverlapped, NativeOverlapped* overlapped, out int errorCode) { Debug.Assert(handle != null, "handle != null"); @@ -347,13 +347,24 @@ internal static unsafe int ReadFileNative(SafeFileHandle handle, Span byte fixed (byte* p = &MemoryMarshal.GetReference(bytes)) { r = overlapped != null ? - Interop.Kernel32.ReadFile(handle, p, bytes.Length, IntPtr.Zero, overlapped) : - Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, IntPtr.Zero); + (syncUsingOverlapped + ? Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, overlapped) + : Interop.Kernel32.ReadFile(handle, p, bytes.Length, IntPtr.Zero, overlapped)) + : Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, IntPtr.Zero); } if (r == 0) { errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(handle); + + if (syncUsingOverlapped && errorCode == Interop.Errors.ERROR_HANDLE_EOF) + { + // https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfile#synchronization-and-file-position : + // "If lpOverlapped is not NULL, then when a synchronous read operation reaches the end of a file, + // ReadFile returns FALSE and GetLastError returns ERROR_HANDLE_EOF" + return numBytesRead; + } + return -1; } else @@ -363,7 +374,7 @@ internal static unsafe int ReadFileNative(SafeFileHandle handle, Span byte } } - internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, NativeOverlapped* overlapped, out int errorCode) + internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, bool syncUsingOverlapped, NativeOverlapped* overlapped, out int errorCode) { Debug.Assert(handle != null, "handle != null"); @@ -373,13 +384,24 @@ internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan= 0; + synchronousSuccess = ReadFileNative(handle, copyBuffer, false, readAwaitable._nativeOverlapped, out errorCode) >= 0; } // If the operation did not synchronously succeed, it either failed or initiated the asynchronous operation. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Windows.cs index 0e236cfa5097f..0fd86c91dd801 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Windows.cs @@ -1079,14 +1079,14 @@ private unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, Nativ { Debug.Assert((_useAsyncIO && overlapped != null) || (!_useAsyncIO && overlapped == null), "Async IO and overlapped parameters inconsistent in call to ReadFileNative."); - return FileStreamHelpers.ReadFileNative(handle, bytes, overlapped, out errorCode); + return FileStreamHelpers.ReadFileNative(handle, bytes, false, overlapped, out errorCode); } private unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, NativeOverlapped* overlapped, out int errorCode) { Debug.Assert((_useAsyncIO && overlapped != null) || (!_useAsyncIO && overlapped == null), "Async IO and overlapped parameters inconsistent in call to WriteFileNative."); - return FileStreamHelpers.WriteFileNative(handle, buffer, overlapped, out errorCode); + return FileStreamHelpers.WriteFileNative(handle, buffer, false, overlapped, out errorCode); } public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs index 5198b512c3a9a..e722867e3b9cc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs @@ -104,7 +104,8 @@ private unsafe int ReadSpan(Span destination) Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); - int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, null, out int errorCode); + NativeOverlapped nativeOverlapped = GetNativeOverlappedForCurrentPosition(); + int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, true, &nativeOverlapped, out int errorCode); if (r == -1) { @@ -136,7 +137,8 @@ private unsafe void WriteSpan(ReadOnlySpan source) Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); - int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, null, out int errorCode); + NativeOverlapped nativeOverlapped = GetNativeOverlappedForCurrentPosition(); + int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, true, &nativeOverlapped, out int errorCode); if (r == -1) { @@ -159,5 +161,15 @@ private unsafe void WriteSpan(ReadOnlySpan source) _filePosition += r; return; } + + private NativeOverlapped GetNativeOverlappedForCurrentPosition() + { + NativeOverlapped nativeOverlapped = default; + // For pipes the offsets are ignored by the OS + nativeOverlapped.OffsetLow = unchecked((int)_filePosition); + nativeOverlapped.OffsetHigh = (int)(_filePosition >> 32); + + return nativeOverlapped; + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs index 654f18bb81de4..15c4581069df2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs @@ -83,7 +83,7 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access public override long Position { get => _filePosition; - set => Seek(value, SeekOrigin.Begin); + set => _filePosition = value; } internal sealed override string Name => _path ?? SR.IO_UnknownFileName; @@ -91,6 +91,8 @@ public override long Position internal sealed override bool IsClosed => _fileHandle.IsClosed; // Flushing is the responsibility of BufferedFileStreamStrategy + // TODO: we might consider calling FileStreamHelpers.Seek(handle, _path, _filePosition, SeekOrigin.Begin); + // here to set the actual file offset internal sealed override SafeFileHandle SafeFileHandle => _fileHandle; // ReadByte and WriteByte methods are used only when the user has disabled buffering on purpose @@ -152,29 +154,33 @@ public sealed override long Seek(long offset, SeekOrigin origin) if (!CanSeek) throw Error.GetSeekNotSupported(); long oldPos = _filePosition; - long pos = SeekCore(_fileHandle, offset, origin); + long pos = origin switch + { + SeekOrigin.Begin => offset, + SeekOrigin.End => FileStreamHelpers.GetFileLength(_fileHandle, _path) + offset, + _ => _filePosition + offset // SeekOrigin.Current + }; + + if (pos >= 0) + { + _filePosition = pos; + } + else + { + // keep throwing the same exception we did when seek was causing actual offset change + throw Win32Marshal.GetExceptionForWin32Error(Interop.Errors.ERROR_INVALID_PARAMETER); + } - // Prevent users from overwriting data in a file that was opened in - // append mode. + // Prevent users from overwriting data in a file that was opened in append mode. if (_appendStart != -1 && pos < _appendStart) { - SeekCore(_fileHandle, oldPos, SeekOrigin.Begin); + _filePosition = oldPos; throw new IOException(SR.IO_SeekAppendOverwrite); } return pos; } - // This doesn't do argument checking. Necessary for SetLength, which must - // set the file pointer beyond the end of the file. This will update the - // internal position - protected long SeekCore(SafeFileHandle fileHandle, long offset, SeekOrigin origin, bool closeInvalidHandle = false) - { - Debug.Assert(!fileHandle.IsClosed && _canSeek, "!fileHandle.IsClosed && _canSeek"); - - return _filePosition = FileStreamHelpers.Seek(fileHandle, _path, offset, origin, closeInvalidHandle); - } - internal sealed override void Lock(long position, long length) => FileStreamHelpers.Lock(_fileHandle, _path, position, length); internal sealed override void Unlock(long position, long length) => FileStreamHelpers.Unlock(_fileHandle, _path, position, length); @@ -192,7 +198,7 @@ private void Init(FileMode mode, string originalPath) // For Append mode... if (mode == FileMode.Append) { - _appendStart = SeekCore(_fileHandle, 0, SeekOrigin.End); + _appendStart = _filePosition = Length; } else { @@ -226,9 +232,15 @@ private void InitFromHandleImpl(SafeFileHandle handle, out bool canSeek, out boo OnInitFromHandle(handle); if (_canSeek) - SeekCore(handle, 0, SeekOrigin.Current); + { + // given strategy was created out of existing handle, so we have to perform + // a syscall to get the current handle offset + _filePosition = FileStreamHelpers.Seek(handle, _path, 0, SeekOrigin.Current); + } else + { _filePosition = 0; + } } public sealed override void SetLength(long value) @@ -239,8 +251,6 @@ public sealed override void SetLength(long value) SetLengthCore(value); } - // We absolutely need this method broken out so that WriteInternalCoreAsync can call - // a method without having to go through buffering code that might call FlushWrite. protected unsafe void SetLengthCore(long value) { Debug.Assert(value >= 0, "value >= 0"); @@ -249,7 +259,7 @@ protected unsafe void SetLengthCore(long value) if (_filePosition > value) { - SeekCore(_fileHandle, 0, SeekOrigin.End); + _filePosition = value; } } }