From d32cf8b6126fc2d7f9a336a63242fc89e221482b Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 14 Aug 2021 21:05:31 +0300 Subject: [PATCH 01/12] Move checking and pinning Windows vectored I/O buffers to a dedicated method. --- .../src/System/IO/RandomAccess.Windows.cs | 96 ++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index d367abb8fb973..67ff29df9aa88 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -4,7 +4,9 @@ using System.Buffers; using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.IO.Strategies; +using System.Numerics; using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; @@ -439,11 +441,103 @@ private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, I return ReadScatterAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); } + // Abstracts away the type signature incompatibility between Memory and ReadOnlyMemory. + private interface IMemoryHandler + { + int GetLength(in T memory); + MemoryHandle Pin(in T memory); + } + + private struct MemoryHandler : IMemoryHandler> + { + public int GetLength(in Memory memory) => memory.Length; + public MemoryHandle Pin(in Memory memory) => memory.Pin(); + } + + private struct ReadOnlyMemoryHandler : IMemoryHandler> + { + public int GetLength(in ReadOnlyMemory memory) => memory.Length; + public MemoryHandle Pin(in ReadOnlyMemory memory) => memory.Pin(); + } + // From https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfilescatter: - // "The file handle must be created with the GENERIC_READ right, and the FILE_FLAG_OVERLAPPED and FILE_FLAG_NO_BUFFERING flags." + // "The file handle must be created with [...] the FILE_FLAG_OVERLAPPED and FILE_FLAG_NO_BUFFERING flags." private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) => handle.IsAsync && ((handle.GetFileOptions() & SafeFileHandle.NoBuffering) != 0); + // From the same source: + // "Each buffer must be at least the size of a system memory page and must be aligned on a system + // memory page size boundary. The system reads/writes one system memory page of data into/from each buffer." + // This method returns true if the buffers can be used by + // the Windows scatter/gather API, which happens when they are: + // 1. aligned at page size boundaries + // 2. exactly one page long each (our own requirement to prevent partial reads) + // 3. not bigger than 2^32 - 1 in total + // This function is also responsible for pinning the buffers if they + // are suitable and they must be unpinned after the I/O operation. + // The total size of the buffers is also returned. + private static bool TryPrepareBuffersForScatterGatherWindowsAPIs(IReadOnlyList buffers, + THandler handler, [NotNullWhen(true)] out MemoryHandle[]? pinnedBuffers, out int totalBytes) + where THandler: struct, IMemoryHandler + { + int pageSize = Environment.SystemPageSize; + Debug.Assert(BitOperations.IsPow2(pageSize), "Page size is not a power of two."); + // We take advantage of the fact that the page size is + // a power of two to avoid an expensive modulo operation. + long alignedAtPageSizeMask = pageSize - 1; + int buffersCount = buffers.Count; + pinnedBuffers = new MemoryHandle[buffersCount]; + + try + { + long totalBytes64 = 0; + for (int i = 0; i < buffersCount; i++) + { + T buffer = buffers[i]; + int length = handler.GetLength(in buffer); + totalBytes64 += length; + if (length != pageSize || totalBytes64 > int.MaxValue) + { + goto Failure; + } + + MemoryHandle handle = pinnedBuffers[i] = handler.Pin(in buffer); + unsafe + { + if (((long)handle.Pointer & alignedAtPageSizeMask) != 0) + { + goto Failure; + } + } + } + + totalBytes = (int)totalBytes64; + return true; + + Failure: + foreach (MemoryHandle handle in pinnedBuffers) + { + handle.Dispose(); + } + + pinnedBuffers = null; + totalBytes = 0; + return false; + } + catch + { + if (pinnedBuffers != null) + { + foreach (MemoryHandle handle in pinnedBuffers) + { + handle.Dispose(); + } + } + + throw; + } + } + private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) { int buffersCount = buffers.Count; From 2d8e82f33f08e5f4f5654f03acf05279333bd388 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 15 Aug 2021 02:20:13 +0300 Subject: [PATCH 02/12] Refactor the scatter/gather APIs to use the common checking method. And use pinned GCHandles and IntPtrs instead of MemoryHandles when passing the segment array to the bottom-most method. --- .../src/System/IO/RandomAccess.Windows.cs | 155 ++++++++++-------- 1 file changed, 85 insertions(+), 70 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 67ff29df9aa88..ead5f410c8fb8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -423,22 +423,38 @@ private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, I return ScheduleSyncReadScatterAtOffsetAsync(handle, buffers, fileOffset, cancellationToken); } - if (CanUseScatterGatherWindowsAPIs(handle)) + switch (buffers.Count) { - long totalBytes = 0; - int buffersCount = buffers.Count; - for (int i = 0; i < buffersCount; i++) + case 0: + return CastValueTask(ReadAtOffsetAsync(handle, Memory.Empty, fileOffset, cancellationToken)); + case 1: + return CastValueTask(ReadAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken)); + } + + if (CanUseScatterGatherWindowsAPIs(handle) + && TryPrepareBuffersForScatterGatherWindowsAPIs(buffers, default(MemoryHandler), out MemoryHandle[]? pinnedBuffers, out int totalBytes)) + { + try { - totalBytes += buffers[i].Length; + return ReadScatterAtOffsetSingleSyscallAsync(handle, pinnedBuffers, fileOffset, totalBytes, + cancellationToken); } - - if (totalBytes <= int.MaxValue) // the ReadFileScatter API uses int, not long + catch { - return ReadScatterAtOffsetSingleSyscallAsync(handle, buffers, fileOffset, (int)totalBytes, cancellationToken); + foreach (MemoryHandle memoryHandle in pinnedBuffers) + { + memoryHandle.Dispose(); + } + + throw; } } return ReadScatterAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); + + static async ValueTask CastValueTask(ValueTask task) => + // we have to await it because we can't cast a VT to VT + await task.ConfigureAwait(false); } // Abstracts away the type signature incompatibility between Memory and ReadOnlyMemory. @@ -538,49 +554,44 @@ private static bool TryPrepareBuffersForScatterGatherWindowsAPIs(IR } } - private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) + private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] pinnedBuffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) { - int buffersCount = buffers.Count; - if (buffersCount == 1) - { - // we have to await it because we can't cast a VT to VT - return await ReadAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken).ConfigureAwait(false); - } - + int buffersCount = pinnedBuffers.Length; // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. " long[] fileSegments = new long[buffersCount + 1]; fileSegments[buffersCount] = 0; - MemoryHandle[] memoryHandles = new MemoryHandle[buffersCount]; - MemoryHandle pinnedSegments = fileSegments.AsMemory().Pin(); + GCHandle pinnedSegments = default; try { + pinnedSegments = GCHandle.Alloc(fileSegments, GCHandleType.Pinned); + for (int i = 0; i < buffersCount; i++) { - Memory buffer = buffers[i]; - MemoryHandle memoryHandle = buffer.Pin(); - memoryHandles[i] = memoryHandle; - unsafe // awaits can't be in an unsafe context { - fileSegments[i] = new IntPtr(memoryHandle.Pointer).ToInt64(); + fileSegments[i] = new IntPtr(pinnedBuffers[i].Pointer).ToInt64(); } } - return await ReadFileScatterAsync(handle, pinnedSegments, totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); + return await ReadFileScatterAsync(handle, pinnedSegments.AddrOfPinnedObject(), totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); } finally { - foreach (MemoryHandle memoryHandle in memoryHandles) + foreach (MemoryHandle memoryHandle in pinnedBuffers) { memoryHandle.Dispose(); } - pinnedSegments.Dispose(); + + if (pinnedSegments.IsAllocated) + { + pinnedSegments.Free(); + } } } - private static unsafe ValueTask ReadFileScatterAsync(SafeFileHandle handle, MemoryHandle pinnedSegments, int bytesToRead, long fileOffset, CancellationToken cancellationToken) + private static unsafe ValueTask ReadFileScatterAsync(SafeFileHandle handle, IntPtr segmentsPtr, int bytesToRead, long fileOffset, CancellationToken cancellationToken) { handle.EnsureThreadPoolBindingInitialized(); @@ -588,9 +599,9 @@ private static unsafe ValueTask ReadFileScatterAsync(SafeFileHandle handle, try { NativeOverlapped* nativeOverlapped = vts.PrepareForOperation(Memory.Empty, fileOffset); - Debug.Assert(pinnedSegments.Pointer != null); + Debug.Assert(segmentsPtr != IntPtr.Zero); - if (Interop.Kernel32.ReadFileScatter(handle, (long*)pinnedSegments.Pointer, bytesToRead, IntPtr.Zero, nativeOverlapped) == 0) + if (Interop.Kernel32.ReadFileScatter(handle, (long*)segmentsPtr, bytesToRead, IntPtr.Zero, nativeOverlapped) == 0) { // The operation failed, or it's pending. int errorCode = FileStreamHelpers.GetLastWin32ErrorAndDisposeHandleIfInvalid(handle); @@ -656,17 +667,30 @@ private static ValueTask WriteGatherAtOffsetAsync(SafeFileHandle handle, IReadOn return ScheduleSyncWriteGatherAtOffsetAsync(handle, buffers, fileOffset, cancellationToken); } - if (CanUseScatterGatherWindowsAPIs(handle)) + switch (buffers.Count) + { + case 0: + return WriteAtOffsetAsync(handle, ReadOnlyMemory.Empty, fileOffset, cancellationToken); + case 1: + return WriteAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken); + } + + if (CanUseScatterGatherWindowsAPIs(handle) + && TryPrepareBuffersForScatterGatherWindowsAPIs(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[]? pinnedBuffers, out int totalBytes)) { - long totalBytes = 0; - for (int i = 0; i < buffers.Count; i++) + try { - totalBytes += buffers[i].Length; + return WriteGatherAtOffsetSingleSyscallAsync(handle, pinnedBuffers, fileOffset, totalBytes, + cancellationToken); } - - if (totalBytes <= int.MaxValue) // the ReadFileScatter API uses int, not long + catch { - return WriteGatherAtOffsetSingleSyscallAsync(handle, buffers, fileOffset, (int)totalBytes, cancellationToken); + foreach (MemoryHandle memoryHandle in pinnedBuffers) + { + memoryHandle.Dispose(); + } + + throw; } } @@ -685,53 +709,44 @@ private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFile } } - private static ValueTask WriteGatherAtOffsetSingleSyscallAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) + private static async ValueTask WriteGatherAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] pinnedBuffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) { - if (buffers.Count == 1) - { - return WriteAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken); - } + // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. " + int buffersCount = pinnedBuffers.Length; + long[] fileSegments = new long[buffersCount + 1]; + fileSegments[buffersCount] = 0; - return Core(handle, buffers, fileOffset, totalBytes, cancellationToken); + GCHandle pinnedSegments = default; - static async ValueTask Core(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) + try { - // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. " - int buffersCount = buffers.Count; - long[] fileSegments = new long[buffersCount + 1]; - fileSegments[buffersCount] = 0; - - MemoryHandle[] memoryHandles = new MemoryHandle[buffersCount]; - MemoryHandle pinnedSegments = fileSegments.AsMemory().Pin(); + pinnedSegments = GCHandle.Alloc(fileSegments, GCHandleType.Pinned); - try + for (int i = 0; i < buffersCount; i++) { - for (int i = 0; i < buffersCount; i++) + unsafe // awaits can't be in an unsafe context { - ReadOnlyMemory buffer = buffers[i]; - MemoryHandle memoryHandle = buffer.Pin(); - memoryHandles[i] = memoryHandle; - - unsafe // awaits can't be in an unsafe context - { - fileSegments[i] = new IntPtr(memoryHandle.Pointer).ToInt64(); - } + fileSegments[i] = new IntPtr(pinnedBuffers[i].Pointer).ToInt64(); } + } - await WriteFileGatherAsync(handle, pinnedSegments, totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); + await WriteFileGatherAsync(handle, pinnedSegments.AddrOfPinnedObject(), totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); + } + finally + { + foreach (MemoryHandle memoryHandle in pinnedBuffers) + { + memoryHandle.Dispose(); } - finally + + if (pinnedSegments.IsAllocated) { - foreach (MemoryHandle memoryHandle in memoryHandles) - { - memoryHandle.Dispose(); - } - pinnedSegments.Dispose(); + pinnedSegments.Free(); } } } - private static unsafe ValueTask WriteFileGatherAsync(SafeFileHandle handle, MemoryHandle pinnedSegments, int bytesToWrite, long fileOffset, CancellationToken cancellationToken) + private static unsafe ValueTask WriteFileGatherAsync(SafeFileHandle handle, IntPtr segmentsPtr, int bytesToWrite, long fileOffset, CancellationToken cancellationToken) { handle.EnsureThreadPoolBindingInitialized(); @@ -739,10 +754,10 @@ private static unsafe ValueTask WriteFileGatherAsync(SafeFileHandle handle, Memo try { NativeOverlapped* nativeOverlapped = vts.PrepareForOperation(ReadOnlyMemory.Empty, fileOffset); - Debug.Assert(pinnedSegments.Pointer != null); + Debug.Assert(segmentsPtr != IntPtr.Zero); // Queue an async WriteFile operation. - if (Interop.Kernel32.WriteFileGather(handle, (long*)pinnedSegments.Pointer, bytesToWrite, IntPtr.Zero, nativeOverlapped) == 0) + if (Interop.Kernel32.WriteFileGather(handle, (long*)segmentsPtr, bytesToWrite, IntPtr.Zero, nativeOverlapped) == 0) { // The operation failed, or it's pending. int errorCode = FileStreamHelpers.GetLastWin32ErrorAndDisposeHandleIfInvalid(handle); From 26e535072ff5d77e8686028e522b13af2e87098e Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 15 Aug 2021 02:22:44 +0300 Subject: [PATCH 03/12] Shorten the name of the buffer-checking method. --- .../src/System/IO/RandomAccess.Windows.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index ead5f410c8fb8..340780a159c4b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -432,7 +432,7 @@ private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, I } if (CanUseScatterGatherWindowsAPIs(handle) - && TryPrepareBuffersForScatterGatherWindowsAPIs(buffers, default(MemoryHandler), out MemoryHandle[]? pinnedBuffers, out int totalBytes)) + && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[]? pinnedBuffers, out int totalBytes)) { try { @@ -490,9 +490,9 @@ private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) // 2. exactly one page long each (our own requirement to prevent partial reads) // 3. not bigger than 2^32 - 1 in total // This function is also responsible for pinning the buffers if they - // are suitable and they must be unpinned after the I/O operation. + // are suitable and they must be unpinned after the I/O operation completes. // The total size of the buffers is also returned. - private static bool TryPrepareBuffersForScatterGatherWindowsAPIs(IReadOnlyList buffers, + private static bool TryPrepareScatterGatherBuffers(IReadOnlyList buffers, THandler handler, [NotNullWhen(true)] out MemoryHandle[]? pinnedBuffers, out int totalBytes) where THandler: struct, IMemoryHandler { @@ -676,7 +676,7 @@ private static ValueTask WriteGatherAtOffsetAsync(SafeFileHandle handle, IReadOn } if (CanUseScatterGatherWindowsAPIs(handle) - && TryPrepareBuffersForScatterGatherWindowsAPIs(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[]? pinnedBuffers, out int totalBytes)) + && TryPrepareScatterGatherBuffers(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[]? pinnedBuffers, out int totalBytes)) { try { From d7b82ffdf1d8d7e7c3e51cdbfd96e018570f1119 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 15 Aug 2021 15:02:19 +0300 Subject: [PATCH 04/12] Directly get the pinned array's address instead of calling GCHandle.AddrOfPinnedObject. --- .../src/System/IO/RandomAccess.Windows.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 340780a159c4b..000f2e5e01faf 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -10,6 +10,7 @@ using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; +using Internal.Runtime.CompilerServices; using Microsoft.Win32.SafeHandles; namespace System.IO @@ -476,6 +477,11 @@ private struct ReadOnlyMemoryHandler : IMemoryHandler> public MemoryHandle Pin(in ReadOnlyMemory memory) => memory.Pin(); } + // GCHandle.AddrOfPinnedObject performs some checks that are not + // necessary since we know we have pinned a specific SZArray. + private static unsafe IntPtr AddrOfPinnedArray(T[] array) => + (IntPtr) Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(array)); + // From https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfilescatter: // "The file handle must be created with [...] the FILE_FLAG_OVERLAPPED and FILE_FLAG_NO_BUFFERING flags." private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) @@ -575,7 +581,7 @@ private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeF } } - return await ReadFileScatterAsync(handle, pinnedSegments.AddrOfPinnedObject(), totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); + return await ReadFileScatterAsync(handle, AddrOfPinnedArray(fileSegments), totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); } finally { @@ -730,7 +736,7 @@ private static async ValueTask WriteGatherAtOffsetSingleSyscallAsync(SafeFileHan } } - await WriteFileGatherAsync(handle, pinnedSegments.AddrOfPinnedObject(), totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); + await WriteFileGatherAsync(handle, AddrOfPinnedArray(fileSegments), totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); } finally { From a160f1d73a13f7c541c6fef24c5c0c3ef119e733 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 15 Aug 2021 18:04:39 +0300 Subject: [PATCH 05/12] Refactor the error handling logic in TryPrepareScatterGatherBuffers. --- .../src/System/IO/RandomAccess.Windows.cs | 23 ++++++------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 000f2e5e01faf..cfc5d5eeece66 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -509,7 +509,9 @@ private static bool TryPrepareScatterGatherBuffers(IReadOnlyList long alignedAtPageSizeMask = pageSize - 1; int buffersCount = buffers.Count; pinnedBuffers = new MemoryHandle[buffersCount]; + totalBytes = 0; + bool success = false; try { long totalBytes64 = 0; @@ -520,7 +522,7 @@ private static bool TryPrepareScatterGatherBuffers(IReadOnlyList totalBytes64 += length; if (length != pageSize || totalBytes64 > int.MaxValue) { - goto Failure; + return false; } MemoryHandle handle = pinnedBuffers[i] = handler.Pin(in buffer); @@ -528,35 +530,24 @@ private static bool TryPrepareScatterGatherBuffers(IReadOnlyList { if (((long)handle.Pointer & alignedAtPageSizeMask) != 0) { - goto Failure; + return false; } } } totalBytes = (int)totalBytes64; + success = true; return true; - - Failure: - foreach (MemoryHandle handle in pinnedBuffers) - { - handle.Dispose(); - } - - pinnedBuffers = null; - totalBytes = 0; - return false; } - catch + finally { - if (pinnedBuffers != null) + if (!success) { foreach (MemoryHandle handle in pinnedBuffers) { handle.Dispose(); } } - - throw; } } From 656adf68f8490e14c7b1a09df4329f9dd4e266d3 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 15 Aug 2021 19:07:04 +0300 Subject: [PATCH 06/12] Allocate the segment array from native memory and at TryPrepareScatterGatherBuffers. --- .../src/System/IO/RandomAccess.Windows.cs | 123 +++++------------- 1 file changed, 33 insertions(+), 90 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index cfc5d5eeece66..6cc4794137ce3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -433,22 +433,9 @@ private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, I } if (CanUseScatterGatherWindowsAPIs(handle) - && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[]? pinnedBuffers, out int totalBytes)) + && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) { - try - { - return ReadScatterAtOffsetSingleSyscallAsync(handle, pinnedBuffers, fileOffset, totalBytes, - cancellationToken); - } - catch - { - foreach (MemoryHandle memoryHandle in pinnedBuffers) - { - memoryHandle.Dispose(); - } - - throw; - } + return ReadScatterAtOffsetSingleSyscallAsync(handle, handlesToDispose, segmentsPtr, fileOffset, totalBytes, cancellationToken); } return ReadScatterAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); @@ -477,11 +464,6 @@ private struct ReadOnlyMemoryHandler : IMemoryHandler> public MemoryHandle Pin(in ReadOnlyMemory memory) => memory.Pin(); } - // GCHandle.AddrOfPinnedObject performs some checks that are not - // necessary since we know we have pinned a specific SZArray. - private static unsafe IntPtr AddrOfPinnedArray(T[] array) => - (IntPtr) Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(array)); - // From https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfilescatter: // "The file handle must be created with [...] the FILE_FLAG_OVERLAPPED and FILE_FLAG_NO_BUFFERING flags." private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) @@ -497,9 +479,10 @@ private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) // 3. not bigger than 2^32 - 1 in total // This function is also responsible for pinning the buffers if they // are suitable and they must be unpinned after the I/O operation completes. - // The total size of the buffers is also returned. - private static bool TryPrepareScatterGatherBuffers(IReadOnlyList buffers, - THandler handler, [NotNullWhen(true)] out MemoryHandle[]? pinnedBuffers, out int totalBytes) + // It also returns a pointer with the segments to be passed to the Windows API and to be + // freed by NativeMemory.Free, and the total size of the buffers that is needed as well. + private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnlyList buffers, + THandler handler, out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes) where THandler: struct, IMemoryHandler { int pageSize = Environment.SystemPageSize; @@ -507,10 +490,16 @@ private static bool TryPrepareScatterGatherBuffers(IReadOnlyList // We take advantage of the fact that the page size is // a power of two to avoid an expensive modulo operation. long alignedAtPageSizeMask = pageSize - 1; + int buffersCount = buffers.Count; - pinnedBuffers = new MemoryHandle[buffersCount]; + handlesToDispose = new MemoryHandle[buffersCount]; + segmentsPtr = IntPtr.Zero; totalBytes = 0; + // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. " + long* segmentsArray = (long*) NativeMemory.Alloc((nuint)(buffersCount + 1), sizeof(long)); + segmentsArray[buffersCount] = 0; + bool success = false; try { @@ -525,16 +514,15 @@ private static bool TryPrepareScatterGatherBuffers(IReadOnlyList return false; } - MemoryHandle handle = pinnedBuffers[i] = handler.Pin(in buffer); - unsafe + MemoryHandle handle = handlesToDispose[i] = handler.Pin(in buffer); + long ptr = segmentsArray[i] = (long)handle.Pointer; + if ((ptr & alignedAtPageSizeMask) != 0) { - if (((long)handle.Pointer & alignedAtPageSizeMask) != 0) - { - return false; - } + return false; } } + segmentsPtr = (IntPtr)segmentsArray; totalBytes = (int)totalBytes64; success = true; return true; @@ -543,47 +531,32 @@ private static bool TryPrepareScatterGatherBuffers(IReadOnlyList { if (!success) { - foreach (MemoryHandle handle in pinnedBuffers) + foreach (MemoryHandle handle in handlesToDispose) { handle.Dispose(); } + + NativeMemory.Free(segmentsArray); } } } - private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] pinnedBuffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) + private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] handlesToDispose, IntPtr segmentsPtr, long fileOffset, int totalBytes, CancellationToken cancellationToken) { - int buffersCount = pinnedBuffers.Length; - // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. " - long[] fileSegments = new long[buffersCount + 1]; - fileSegments[buffersCount] = 0; - - GCHandle pinnedSegments = default; - try { - pinnedSegments = GCHandle.Alloc(fileSegments, GCHandleType.Pinned); - - for (int i = 0; i < buffersCount; i++) - { - unsafe // awaits can't be in an unsafe context - { - fileSegments[i] = new IntPtr(pinnedBuffers[i].Pointer).ToInt64(); - } - } - - return await ReadFileScatterAsync(handle, AddrOfPinnedArray(fileSegments), totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); + return await ReadFileScatterAsync(handle, segmentsPtr, totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); } finally { - foreach (MemoryHandle memoryHandle in pinnedBuffers) + foreach (MemoryHandle memoryHandle in handlesToDispose) { memoryHandle.Dispose(); } - if (pinnedSegments.IsAllocated) + unsafe { - pinnedSegments.Free(); + NativeMemory.Free(segmentsPtr.ToPointer()); } } } @@ -673,22 +646,9 @@ private static ValueTask WriteGatherAtOffsetAsync(SafeFileHandle handle, IReadOn } if (CanUseScatterGatherWindowsAPIs(handle) - && TryPrepareScatterGatherBuffers(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[]? pinnedBuffers, out int totalBytes)) + && TryPrepareScatterGatherBuffers(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) { - try - { - return WriteGatherAtOffsetSingleSyscallAsync(handle, pinnedBuffers, fileOffset, totalBytes, - cancellationToken); - } - catch - { - foreach (MemoryHandle memoryHandle in pinnedBuffers) - { - memoryHandle.Dispose(); - } - - throw; - } + return WriteGatherAtOffsetSingleSyscallAsync(handle, handlesToDispose, segmentsPtr, fileOffset, totalBytes, cancellationToken); } return WriteGatherAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); @@ -706,39 +666,22 @@ private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFile } } - private static async ValueTask WriteGatherAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] pinnedBuffers, long fileOffset, int totalBytes, CancellationToken cancellationToken) + private static async ValueTask WriteGatherAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] handlesToDispose, IntPtr segmentsPtr, long fileOffset, int totalBytes, CancellationToken cancellationToken) { - // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. " - int buffersCount = pinnedBuffers.Length; - long[] fileSegments = new long[buffersCount + 1]; - fileSegments[buffersCount] = 0; - - GCHandle pinnedSegments = default; - try { - pinnedSegments = GCHandle.Alloc(fileSegments, GCHandleType.Pinned); - - for (int i = 0; i < buffersCount; i++) - { - unsafe // awaits can't be in an unsafe context - { - fileSegments[i] = new IntPtr(pinnedBuffers[i].Pointer).ToInt64(); - } - } - - await WriteFileGatherAsync(handle, AddrOfPinnedArray(fileSegments), totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); + await WriteFileGatherAsync(handle, segmentsPtr, totalBytes, fileOffset, cancellationToken).ConfigureAwait(false); } finally { - foreach (MemoryHandle memoryHandle in pinnedBuffers) + foreach (MemoryHandle memoryHandle in handlesToDispose) { memoryHandle.Dispose(); } - if (pinnedSegments.IsAllocated) + unsafe { - pinnedSegments.Free(); + NativeMemory.Free(segmentsPtr.ToPointer()); } } } From e1b94a9323c554dc7e492b291d451072da1cc4c1 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 15 Aug 2021 19:07:53 +0300 Subject: [PATCH 07/12] Cache the page size on a static readonly field and add a couple of TODOs. --- .../src/System/IO/RandomAccess.Windows.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 6cc4794137ce3..90a6c4de074bf 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -446,6 +446,7 @@ static async ValueTask CastValueTask(ValueTask task) => } // Abstracts away the type signature incompatibility between Memory and ReadOnlyMemory. + // TODO: Use abstract static methods when they become stable. private interface IMemoryHandler { int GetLength(in T memory); @@ -469,6 +470,9 @@ private struct ReadOnlyMemoryHandler : IMemoryHandler> private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) => handle.IsAsync && ((handle.GetFileOptions() & SafeFileHandle.NoBuffering) != 0); + // TODO: Use SystemPageSize directly when #57442 is fixed. + private static readonly int s_cachedPageSize = Environment.SystemPageSize; + // From the same source: // "Each buffer must be at least the size of a system memory page and must be aligned on a system // memory page size boundary. The system reads/writes one system memory page of data into/from each buffer." @@ -485,7 +489,7 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly THandler handler, out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes) where THandler: struct, IMemoryHandler { - int pageSize = Environment.SystemPageSize; + int pageSize = s_cachedPageSize; Debug.Assert(BitOperations.IsPow2(pageSize), "Page size is not a power of two."); // We take advantage of the fact that the page size is // a power of two to avoid an expensive modulo operation. From 92924d801000dc10c72851fd58c8b590c397e2c3 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Wed, 18 Aug 2021 19:42:19 +0300 Subject: [PATCH 08/12] Make the memory handlers readonly structs. --- .../src/System/IO/RandomAccess.Windows.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 90a6c4de074bf..bc6a39e3099d5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -453,13 +453,13 @@ private interface IMemoryHandler MemoryHandle Pin(in T memory); } - private struct MemoryHandler : IMemoryHandler> + private readonly struct MemoryHandler : IMemoryHandler> { public int GetLength(in Memory memory) => memory.Length; public MemoryHandle Pin(in Memory memory) => memory.Pin(); } - private struct ReadOnlyMemoryHandler : IMemoryHandler> + private readonly struct ReadOnlyMemoryHandler : IMemoryHandler> { public int GetLength(in ReadOnlyMemory memory) => memory.Length; public MemoryHandle Pin(in ReadOnlyMemory memory) => memory.Pin(); From b1d60b68b88b5bb404f496ccf04c2ca17c5f9746 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Wed, 18 Aug 2021 19:47:16 +0300 Subject: [PATCH 09/12] Add a test. --- .../tests/RandomAccess/NoBuffering.Windows.cs | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs index d897cc7b8be25..17bb0ba370376 100644 --- a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs +++ b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs @@ -176,5 +176,36 @@ public async Task WriteAsyncUsingMultipleBuffers(bool async) Assert.Equal(content, File.ReadAllBytes(filePath)); } + + [Fact] + public async Task ReadWriteAsyncUsingNonPageSizedMultipleBuffers() + { + string filePath = GetTestFilePath(); + // The Windows scatter/gather APIs accept segments that are exactly one page long. + // Combined with the FILE_FLAG_NO_BUFFERING's requirements, the segments must also + // be aligned at page size boundaries and have a size of a multiple of the page size. + // Using segments with a length of twice the page size adheres to the second requirement + // but not the first. The RandomAccess implementation will see it and issue sequential + // read/write syscalls per segment, instead of one scatter/gather syscall. + // This test verifies that fallback behavior. + int bufferSize = Environment.SystemPageSize * 2; + int fileSize = bufferSize * 2; + byte[] content = RandomNumberGenerator.GetBytes(fileSize); + + using (SafeFileHandle handle = File.OpenHandle(filePath, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.Asynchronous | NoBuffering)) + using (SectorAlignedMemory buffer = SectorAlignedMemory.Allocate(fileSize)) + { + Memory firstHalf = buffer.Memory.Slice(0, bufferSize); + Memory secondHalf = buffer.Memory.Slice(bufferSize); + + content.AsSpan().CopyTo(buffer.GetSpan()); + await RandomAccess.WriteAsync(handle, new ReadOnlyMemory[] { firstHalf, secondHalf }, 0); + + buffer.GetSpan().Clear(); + await RandomAccess.ReadAsync(handle, new Memory[] { firstHalf, secondHalf }, 0); + } + + Assert.Equal(content, await File.ReadAllBytesAsync(filePath)); + } } } From 749b6c7208bf4745e101990f5de9a27e2e83252f Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 19 Aug 2021 15:22:28 +0300 Subject: [PATCH 10/12] Reorder some methods with PR feedback taken into consideration. --- .../src/System/IO/RandomAccess.Windows.cs | 130 +++++++++--------- 1 file changed, 64 insertions(+), 66 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index bc6a39e3099d5..5987fe028ea54 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -4,13 +4,11 @@ using System.Buffers; using System.Collections.Generic; using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; using System.IO.Strategies; using System.Numerics; using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; -using Internal.Runtime.CompilerServices; using Microsoft.Win32.SafeHandles; namespace System.IO @@ -19,6 +17,9 @@ public static partial class RandomAccess { private static readonly IOCompletionCallback s_callback = AllocateCallback(); + // TODO: Use SystemPageSize directly when #57442 is fixed. + private static readonly int s_cachedPageSize = Environment.SystemPageSize; + internal static unsafe long GetFileLength(SafeFileHandle handle) { Interop.Kernel32.FILE_STANDARD_INFO info; @@ -416,63 +417,11 @@ internal static void WriteGatherAtOffset(SafeFileHandle handle, IReadOnlyList ReadScatterAtOffsetAsync(SafeFileHandle handle, IReadOnlyList> buffers, - long fileOffset, CancellationToken cancellationToken) - { - if (!handle.IsAsync) - { - return ScheduleSyncReadScatterAtOffsetAsync(handle, buffers, fileOffset, cancellationToken); - } - - switch (buffers.Count) - { - case 0: - return CastValueTask(ReadAtOffsetAsync(handle, Memory.Empty, fileOffset, cancellationToken)); - case 1: - return CastValueTask(ReadAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken)); - } - - if (CanUseScatterGatherWindowsAPIs(handle) - && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) - { - return ReadScatterAtOffsetSingleSyscallAsync(handle, handlesToDispose, segmentsPtr, fileOffset, totalBytes, cancellationToken); - } - - return ReadScatterAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); - - static async ValueTask CastValueTask(ValueTask task) => - // we have to await it because we can't cast a VT to VT - await task.ConfigureAwait(false); - } - - // Abstracts away the type signature incompatibility between Memory and ReadOnlyMemory. - // TODO: Use abstract static methods when they become stable. - private interface IMemoryHandler - { - int GetLength(in T memory); - MemoryHandle Pin(in T memory); - } - - private readonly struct MemoryHandler : IMemoryHandler> - { - public int GetLength(in Memory memory) => memory.Length; - public MemoryHandle Pin(in Memory memory) => memory.Pin(); - } - - private readonly struct ReadOnlyMemoryHandler : IMemoryHandler> - { - public int GetLength(in ReadOnlyMemory memory) => memory.Length; - public MemoryHandle Pin(in ReadOnlyMemory memory) => memory.Pin(); - } - // From https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfilescatter: // "The file handle must be created with [...] the FILE_FLAG_OVERLAPPED and FILE_FLAG_NO_BUFFERING flags." private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) => handle.IsAsync && ((handle.GetFileOptions() & SafeFileHandle.NoBuffering) != 0); - // TODO: Use SystemPageSize directly when #57442 is fixed. - private static readonly int s_cachedPageSize = Environment.SystemPageSize; - // From the same source: // "Each buffer must be at least the size of a system memory page and must be aligned on a system // memory page size boundary. The system reads/writes one system memory page of data into/from each buffer." @@ -545,6 +494,35 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly } } + private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, IReadOnlyList> buffers, + long fileOffset, CancellationToken cancellationToken) + { + if (!handle.IsAsync) + { + return ScheduleSyncReadScatterAtOffsetAsync(handle, buffers, fileOffset, cancellationToken); + } + + switch (buffers.Count) + { + case 0: + return CastValueTask(ReadAtOffsetAsync(handle, Memory.Empty, fileOffset, cancellationToken)); + case 1: + return CastValueTask(ReadAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken)); + } + + if (CanUseScatterGatherWindowsAPIs(handle) + && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) + { + return ReadScatterAtOffsetSingleSyscallAsync(handle, handlesToDispose, segmentsPtr, fileOffset, totalBytes, cancellationToken); + } + + return ReadScatterAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); + + static async ValueTask CastValueTask(ValueTask task) => + // we have to await it because we can't cast a VT to VT + await task.ConfigureAwait(false); + } + private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] handlesToDispose, IntPtr segmentsPtr, long fileOffset, int totalBytes, CancellationToken cancellationToken) { try @@ -658,18 +636,6 @@ private static ValueTask WriteGatherAtOffsetAsync(SafeFileHandle handle, IReadOn return WriteGatherAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); } - private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, CancellationToken cancellationToken) - { - long bytesWritten = 0; - int buffersCount = buffers.Count; - for (int i = 0; i < buffersCount; i++) - { - ReadOnlyMemory rom = buffers[i]; - await WriteAtOffsetAsync(handle, rom, fileOffset + bytesWritten, cancellationToken).ConfigureAwait(false); - bytesWritten += rom.Length; - } - } - private static async ValueTask WriteGatherAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] handlesToDispose, IntPtr segmentsPtr, long fileOffset, int totalBytes, CancellationToken cancellationToken) { try @@ -732,6 +698,18 @@ private static unsafe ValueTask WriteFileGatherAsync(SafeFileHandle handle, IntP return new ValueTask(vts, vts.Version); } + private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, CancellationToken cancellationToken) + { + long bytesWritten = 0; + int buffersCount = buffers.Count; + for (int i = 0; i < buffersCount; i++) + { + ReadOnlyMemory rom = buffers[i]; + await WriteAtOffsetAsync(handle, rom, fileOffset + bytesWritten, cancellationToken).ConfigureAwait(false); + bytesWritten += rom.Length; + } + } + private static unsafe NativeOverlapped* GetNativeOverlappedForAsyncHandle(ThreadPoolBoundHandle threadPoolBinding, long fileOffset, CallbackResetEvent resetEvent) { // After SafeFileHandle is bound to ThreadPool, we need to use ThreadPoolBinding @@ -803,5 +781,25 @@ internal unsafe void FreeNativeOverlapped(NativeOverlapped* pOverlapped) } } } + + // Abstracts away the type signature incompatibility between Memory and ReadOnlyMemory. + // TODO: Use abstract static methods when they become stable. + private interface IMemoryHandler + { + int GetLength(in T memory); + MemoryHandle Pin(in T memory); + } + + private readonly struct MemoryHandler : IMemoryHandler> + { + public int GetLength(in Memory memory) => memory.Length; + public MemoryHandle Pin(in Memory memory) => memory.Pin(); + } + + private readonly struct ReadOnlyMemoryHandler : IMemoryHandler> + { + public int GetLength(in ReadOnlyMemory memory) => memory.Length; + public MemoryHandle Pin(in ReadOnlyMemory memory) => memory.Pin(); + } } } From 5ed633d0d50c6b26cdbf4293209ed7297a36979a Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Fri, 20 Aug 2021 10:45:56 +0300 Subject: [PATCH 11/12] Stop special-casing scatter/gather operations with zero or one buffer. --- .../src/System/IO/RandomAccess.Windows.cs | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 5987fe028ea54..bdc8e470a9c35 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -502,14 +502,6 @@ private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, I return ScheduleSyncReadScatterAtOffsetAsync(handle, buffers, fileOffset, cancellationToken); } - switch (buffers.Count) - { - case 0: - return CastValueTask(ReadAtOffsetAsync(handle, Memory.Empty, fileOffset, cancellationToken)); - case 1: - return CastValueTask(ReadAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken)); - } - if (CanUseScatterGatherWindowsAPIs(handle) && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) { @@ -517,10 +509,6 @@ private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, I } return ReadScatterAtOffsetMultipleSyscallsAsync(handle, buffers, fileOffset, cancellationToken); - - static async ValueTask CastValueTask(ValueTask task) => - // we have to await it because we can't cast a VT to VT - await task.ConfigureAwait(false); } private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeFileHandle handle, MemoryHandle[] handlesToDispose, IntPtr segmentsPtr, long fileOffset, int totalBytes, CancellationToken cancellationToken) @@ -619,14 +607,6 @@ private static ValueTask WriteGatherAtOffsetAsync(SafeFileHandle handle, IReadOn return ScheduleSyncWriteGatherAtOffsetAsync(handle, buffers, fileOffset, cancellationToken); } - switch (buffers.Count) - { - case 0: - return WriteAtOffsetAsync(handle, ReadOnlyMemory.Empty, fileOffset, cancellationToken); - case 1: - return WriteAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken); - } - if (CanUseScatterGatherWindowsAPIs(handle) && TryPrepareScatterGatherBuffers(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)) { From 7e75cc776ef30f474971af1a330d388cdbff9c9b Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 23 Aug 2021 19:45:06 +0300 Subject: [PATCH 12/12] Factor the cleaning-up of the segment buffers into a separate method. --- .../src/System/IO/RandomAccess.Windows.cs | 51 ++++++++----------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index bdc8e470a9c35..31adaca38fe5e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -432,8 +432,10 @@ private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle) // 3. not bigger than 2^32 - 1 in total // This function is also responsible for pinning the buffers if they // are suitable and they must be unpinned after the I/O operation completes. - // It also returns a pointer with the segments to be passed to the Windows API and to be - // freed by NativeMemory.Free, and the total size of the buffers that is needed as well. + // It also returns a pointer with the segments to be passed to the + // Windows API, and the total size of the buffers that is needed as well. + // The pinned MemoryHandles and the pointer to the segments must be cleaned-up + // with the CleanupScatterGatherBuffers method. private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnlyList buffers, THandler handler, out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes) where THandler: struct, IMemoryHandler @@ -450,8 +452,8 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly totalBytes = 0; // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. " - long* segmentsArray = (long*) NativeMemory.Alloc((nuint)(buffersCount + 1), sizeof(long)); - segmentsArray[buffersCount] = 0; + long* segments = (long*) NativeMemory.Alloc((nuint)(buffersCount + 1), sizeof(long)); + segments[buffersCount] = 0; bool success = false; try @@ -468,14 +470,14 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly } MemoryHandle handle = handlesToDispose[i] = handler.Pin(in buffer); - long ptr = segmentsArray[i] = (long)handle.Pointer; + long ptr = segments[i] = (long)handle.Pointer; if ((ptr & alignedAtPageSizeMask) != 0) { return false; } } - segmentsPtr = (IntPtr)segmentsArray; + segmentsPtr = (IntPtr)segments; totalBytes = (int)totalBytes64; success = true; return true; @@ -484,16 +486,21 @@ private static unsafe bool TryPrepareScatterGatherBuffers(IReadOnly { if (!success) { - foreach (MemoryHandle handle in handlesToDispose) - { - handle.Dispose(); - } - - NativeMemory.Free(segmentsArray); + CleanupScatterGatherBuffers(handlesToDispose, (IntPtr) segments); } } } + private static unsafe void CleanupScatterGatherBuffers(MemoryHandle[] handlesToDispose, IntPtr segmentsPtr) + { + foreach (MemoryHandle handle in handlesToDispose) + { + handle.Dispose(); + } + + NativeMemory.Free((void*) segmentsPtr); + } + private static ValueTask ReadScatterAtOffsetAsync(SafeFileHandle handle, IReadOnlyList> buffers, long fileOffset, CancellationToken cancellationToken) { @@ -519,15 +526,7 @@ private static async ValueTask ReadScatterAtOffsetSingleSyscallAsync(SafeF } finally { - foreach (MemoryHandle memoryHandle in handlesToDispose) - { - memoryHandle.Dispose(); - } - - unsafe - { - NativeMemory.Free(segmentsPtr.ToPointer()); - } + CleanupScatterGatherBuffers(handlesToDispose, segmentsPtr); } } @@ -624,15 +623,7 @@ private static async ValueTask WriteGatherAtOffsetSingleSyscallAsync(SafeFileHan } finally { - foreach (MemoryHandle memoryHandle in handlesToDispose) - { - memoryHandle.Dispose(); - } - - unsafe - { - NativeMemory.Free(segmentsPtr.ToPointer()); - } + CleanupScatterGatherBuffers(handlesToDispose, segmentsPtr); } }