From fb5ea4bbacd83972cbcf704d1a0622d50cab5d48 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 7 Dec 2020 19:26:59 +0100 Subject: [PATCH 01/17] use ArrayPool instead of having a private buffer cache --- .../Diagnostics/ProcessManager.Win32.cs | 78 +++++++++---------- 1 file changed, 35 insertions(+), 43 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 18243a75c56702..03b9a05d35aff7 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -242,9 +242,6 @@ private static void HandleLastWin32Error() internal static class NtProcessInfoHelper { - // Cache a single buffer for use in GetProcessInfos(). - private static long[]? CachedBuffer; - // Use a smaller buffer size on debug to ensure we hit the retry path. #if DEBUG private const int DefaultCachedBufferSize = 1024; @@ -257,58 +254,53 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = ProcessInfo[] processInfos; // Start with the default buffer size. - int bufferSize = DefaultCachedBufferSize; + int bufferSize = DefaultCachedBufferSize * sizeof(long); // Get the cached buffer. - long[]? buffer = Interlocked.Exchange(ref CachedBuffer, null); + byte[] buffer = ArrayPool.Shared.Rent(bufferSize); - try + while (true) { - while (true) - { - if (buffer == null) - { - // Allocate buffer of longs since some platforms require the buffer to be 64-bit aligned. - buffer = new long[(bufferSize + 7) / 8]; - } + uint requiredSize = 0; - uint requiredSize = 0; - - unsafe + unsafe + { + // Note that the buffer will contain pointers to itself and it needs to be pinned while it is being processed + // by GetProcessInfos below + fixed (byte* bufferPtr = buffer) { - // Note that the buffer will contain pointers to itself and it needs to be pinned while it is being processed - // by GetProcessInfos below - fixed (long* bufferPtr = buffer) + // some platforms require the buffer to be 64-bit aligned. + // ArrayPool does not guarantee 64-byte alignment, so we take the address of first 64-byte aligned element + // we round up the address of the pinned array to the nearest multiple of 64 + Debug.Assert(bufferSize > 64); + byte* alignedBufferPtr = (byte*)(((nint)bufferPtr + 63) & ~63); + int firstAlignedElementIndex = (int)(alignedBufferPtr - bufferPtr); + + uint status = Interop.NtDll.NtQuerySystemInformation( + Interop.NtDll.SystemProcessInformation, + alignedBufferPtr, + (uint)(buffer.Length - firstAlignedElementIndex), + &requiredSize); + + if (status != Interop.NtDll.STATUS_INFO_LENGTH_MISMATCH) { - uint status = Interop.NtDll.NtQuerySystemInformation( - Interop.NtDll.SystemProcessInformation, - bufferPtr, - (uint)(buffer.Length * sizeof(long)), - &requiredSize); - - if (status != Interop.NtDll.STATUS_INFO_LENGTH_MISMATCH) + // see definition of NT_SUCCESS(Status) in SDK + if ((int)status < 0) { - // see definition of NT_SUCCESS(Status) in SDK - if ((int)status < 0) - { - throw new InvalidOperationException(SR.CouldntGetProcessInfos, new Win32Exception((int)status)); - } - - // Parse the data block to get process information - processInfos = GetProcessInfos(MemoryMarshal.AsBytes(buffer), processIdFilter); - break; + ArrayPool.Shared.Return(buffer); + + throw new InvalidOperationException(SR.CouldntGetProcessInfos, new Win32Exception((int)status)); } + + // Parse the data block to get process information + processInfos = GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex), processIdFilter); + break; } } - - buffer = null; - bufferSize = GetNewBufferSize(bufferSize, (int)requiredSize); } - } - finally - { - // Cache the final buffer for use on the next call. - Interlocked.Exchange(ref CachedBuffer, buffer); + + ArrayPool.Shared.Return(buffer); + bufferSize = GetNewBufferSize(bufferSize, (int)requiredSize); } return processInfos; From d7cf90231b9364e812f660dc1b6206232485bef0 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 7 Dec 2020 20:18:10 +0100 Subject: [PATCH 02/17] include 8 in the const, dont use sizeof(long) to calculate it --- .../src/System/Diagnostics/ProcessManager.Win32.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 03b9a05d35aff7..c23e0e30439996 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -244,9 +244,9 @@ internal static class NtProcessInfoHelper { // Use a smaller buffer size on debug to ensure we hit the retry path. #if DEBUG - private const int DefaultCachedBufferSize = 1024; + private const int DefaultCachedBufferSize = 8 * 1024; #else - private const int DefaultCachedBufferSize = 128 * 1024; + private const int DefaultCachedBufferSize = 8 * 128 * 1024; #endif internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = null) @@ -254,7 +254,7 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = ProcessInfo[] processInfos; // Start with the default buffer size. - int bufferSize = DefaultCachedBufferSize * sizeof(long); + int bufferSize = DefaultCachedBufferSize; // Get the cached buffer. byte[] buffer = ArrayPool.Shared.Rent(bufferSize); From bed319f231cbcb14bcd581b260d562d62d0e277c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 7 Dec 2020 20:18:51 +0100 Subject: [PATCH 03/17] fix a bug --- .../src/System/Diagnostics/ProcessManager.Win32.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index c23e0e30439996..a1712fe8b5893d 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -256,13 +256,13 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = // Start with the default buffer size. int bufferSize = DefaultCachedBufferSize; - // Get the cached buffer. - byte[] buffer = ArrayPool.Shared.Rent(bufferSize); - while (true) { uint requiredSize = 0; + // Get the cached buffer. + byte[] buffer = ArrayPool.Shared.Rent(bufferSize); + unsafe { // Note that the buffer will contain pointers to itself and it needs to be pinned while it is being processed From 98bdfb34d5765636b2e69e92d2bbcd57584bba0b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 7 Dec 2020 20:31:15 +0100 Subject: [PATCH 04/17] move the return of array to the pool to a finally block --- .../Diagnostics/ProcessManager.Win32.cs | 60 ++++++++++--------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index a1712fe8b5893d..e48c9fa5f3545b 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -263,43 +263,47 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = // Get the cached buffer. byte[] buffer = ArrayPool.Shared.Rent(bufferSize); - unsafe + try { - // Note that the buffer will contain pointers to itself and it needs to be pinned while it is being processed - // by GetProcessInfos below - fixed (byte* bufferPtr = buffer) + unsafe { - // some platforms require the buffer to be 64-bit aligned. - // ArrayPool does not guarantee 64-byte alignment, so we take the address of first 64-byte aligned element - // we round up the address of the pinned array to the nearest multiple of 64 - Debug.Assert(bufferSize > 64); - byte* alignedBufferPtr = (byte*)(((nint)bufferPtr + 63) & ~63); - int firstAlignedElementIndex = (int)(alignedBufferPtr - bufferPtr); - - uint status = Interop.NtDll.NtQuerySystemInformation( - Interop.NtDll.SystemProcessInformation, - alignedBufferPtr, - (uint)(buffer.Length - firstAlignedElementIndex), - &requiredSize); - - if (status != Interop.NtDll.STATUS_INFO_LENGTH_MISMATCH) + // Note that the buffer will contain pointers to itself and it needs to be pinned while it is being processed + // by GetProcessInfos below + fixed (byte* bufferPtr = buffer) { - // see definition of NT_SUCCESS(Status) in SDK - if ((int)status < 0) + // some platforms require the buffer to be 64-bit aligned. + // ArrayPool does not guarantee 64-byte alignment, so we take the address of first 64-byte aligned element + // we round up the address of the pinned array to the nearest multiple of 64 + Debug.Assert(bufferSize > 64); + byte* alignedBufferPtr = (byte*)(((nint)bufferPtr + 63) & ~63); + int firstAlignedElementIndex = (int)(alignedBufferPtr - bufferPtr); + + uint status = Interop.NtDll.NtQuerySystemInformation( + Interop.NtDll.SystemProcessInformation, + alignedBufferPtr, + (uint)(buffer.Length - firstAlignedElementIndex), + &requiredSize); + + if (status != Interop.NtDll.STATUS_INFO_LENGTH_MISMATCH) { - ArrayPool.Shared.Return(buffer); - - throw new InvalidOperationException(SR.CouldntGetProcessInfos, new Win32Exception((int)status)); + // see definition of NT_SUCCESS(Status) in SDK + if ((int)status < 0) + { + throw new InvalidOperationException(SR.CouldntGetProcessInfos, new Win32Exception((int)status)); + } + + // Parse the data block to get process information + processInfos = GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex), processIdFilter); + break; } - - // Parse the data block to get process information - processInfos = GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex), processIdFilter); - break; } } } + finally + { + ArrayPool.Shared.Return(buffer); + } - ArrayPool.Shared.Return(buffer); bufferSize = GetNewBufferSize(bufferSize, (int)requiredSize); } From 0b74be502d15ad078966157d8628d43aa92076f8 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 8 Dec 2020 10:12:37 +0100 Subject: [PATCH 05/17] 64 bit not 64 byte alignment --- .../src/System/Diagnostics/ProcessManager.Win32.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index e48c9fa5f3545b..0608bc30cf339b 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -272,10 +272,10 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = fixed (byte* bufferPtr = buffer) { // some platforms require the buffer to be 64-bit aligned. - // ArrayPool does not guarantee 64-byte alignment, so we take the address of first 64-byte aligned element + // ArrayPool does not guarantee 64-bit alignment, so we take the address of first 64-bit aligned element // we round up the address of the pinned array to the nearest multiple of 64 - Debug.Assert(bufferSize > 64); - byte* alignedBufferPtr = (byte*)(((nint)bufferPtr + 63) & ~63); + Debug.Assert(bufferSize > 8); + byte* alignedBufferPtr = (byte*)(((nint)bufferPtr + 7) & ~7); int firstAlignedElementIndex = (int)(alignedBufferPtr - bufferPtr); uint status = Interop.NtDll.NtQuerySystemInformation( From 7e5aa44fbdc4f2d37106224d1f1b6f1c157eb145 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 8 Dec 2020 18:18:09 +0100 Subject: [PATCH 06/17] store information about most recent size --- .../src/System/Diagnostics/ProcessManager.Win32.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 0608bc30cf339b..e21792bab35e33 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -249,12 +249,14 @@ internal static class NtProcessInfoHelper private const int DefaultCachedBufferSize = 8 * 128 * 1024; #endif + private static int MostRecentSize = DefaultCachedBufferSize; + internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = null) { ProcessInfo[] processInfos; // Start with the default buffer size. - int bufferSize = DefaultCachedBufferSize; + int bufferSize = MostRecentSize; while (true) { @@ -294,6 +296,7 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = // Parse the data block to get process information processInfos = GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex), processIdFilter); + MostRecentSize = buffer.Length; break; } } From f1dc73a49fa1de18cf0f4f866aa916b2eefaf939 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 8 Dec 2020 19:07:31 +0100 Subject: [PATCH 07/17] simplify the const --- .../src/System/Diagnostics/ProcessManager.Win32.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index e21792bab35e33..7bd47819fc6541 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -246,7 +246,7 @@ internal static class NtProcessInfoHelper #if DEBUG private const int DefaultCachedBufferSize = 8 * 1024; #else - private const int DefaultCachedBufferSize = 8 * 128 * 1024; + private const int DefaultCachedBufferSize = 1024 * 1024; #endif private static int MostRecentSize = DefaultCachedBufferSize; From 5b052a0458ff7427a7813517cc4d61ee83c12f11 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 9 Dec 2020 12:23:23 +0100 Subject: [PATCH 08/17] dont use ArrayPool for big buffers (it would allocate expensive Gen 2 objects), switch to Marshal.AllocHGlobal instead --- .../Diagnostics/ProcessManager.Win32.cs | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 7bd47819fc6541..9bd401116bd354 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -248,6 +248,8 @@ internal static class NtProcessInfoHelper #else private const int DefaultCachedBufferSize = 1024 * 1024; #endif + // use DefaultCachedBufferSize to ensure we test the unmanaged memory code path path at least in Debug builds + private const int RentSizeLimit = DefaultCachedBufferSize + 1; private static int MostRecentSize = DefaultCachedBufferSize; @@ -262,13 +264,18 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = { uint requiredSize = 0; - // Get the cached buffer. - byte[] buffer = ArrayPool.Shared.Rent(bufferSize); + (byte[]? managed, IntPtr unmanaged) = bufferSize < RentSizeLimit + ? (ArrayPool.Shared.Rent(bufferSize), IntPtr.Zero) + : (null, Marshal.AllocHGlobal(bufferSize)); + + Debug.Assert(managed != null || unmanaged != IntPtr.Zero); try { unsafe { + Span buffer = managed != null ? managed : new Span(unmanaged.ToPointer(), bufferSize); + // Note that the buffer will contain pointers to itself and it needs to be pinned while it is being processed // by GetProcessInfos below fixed (byte* bufferPtr = buffer) @@ -295,7 +302,7 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = } // Parse the data block to get process information - processInfos = GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex), processIdFilter); + processInfos = GetProcessInfos(buffer.Slice(firstAlignedElementIndex), processIdFilter); MostRecentSize = buffer.Length; break; } @@ -304,7 +311,14 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = } finally { - ArrayPool.Shared.Return(buffer); + if (managed != null) + { + ArrayPool.Shared.Return(managed); + } + else + { + Marshal.FreeHGlobal(unmanaged); + } } bufferSize = GetNewBufferSize(bufferSize, (int)requiredSize); From 375543f3bdce8548271cdaafc76f66895df9eaf9 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 9 Dec 2020 12:46:23 +0100 Subject: [PATCH 09/17] minor refactor --- .../Diagnostics/ProcessManager.Win32.cs | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 9bd401116bd354..114b527283ca27 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -255,20 +255,16 @@ internal static class NtProcessInfoHelper internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = null) { - ProcessInfo[] processInfos; - // Start with the default buffer size. int bufferSize = MostRecentSize; while (true) { - uint requiredSize = 0; - (byte[]? managed, IntPtr unmanaged) = bufferSize < RentSizeLimit ? (ArrayPool.Shared.Rent(bufferSize), IntPtr.Zero) : (null, Marshal.AllocHGlobal(bufferSize)); - Debug.Assert(managed != null || unmanaged != IntPtr.Zero); + Debug.Assert((managed != null && unmanaged == IntPtr.Zero) || (managed == null && unmanaged != IntPtr.Zero)); try { @@ -287,6 +283,7 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = byte* alignedBufferPtr = (byte*)(((nint)bufferPtr + 7) & ~7); int firstAlignedElementIndex = (int)(alignedBufferPtr - bufferPtr); + uint requiredSize = 0; uint status = Interop.NtDll.NtQuerySystemInformation( Interop.NtDll.SystemProcessInformation, alignedBufferPtr, @@ -301,11 +298,12 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = throw new InvalidOperationException(SR.CouldntGetProcessInfos, new Win32Exception((int)status)); } - // Parse the data block to get process information - processInfos = GetProcessInfos(buffer.Slice(firstAlignedElementIndex), processIdFilter); MostRecentSize = buffer.Length; - break; + // Parse the data block to get process information + return GetProcessInfos(buffer.Slice(firstAlignedElementIndex), processIdFilter); } + + bufferSize = GetNewBufferSize(bufferSize, (int)requiredSize); } } } @@ -320,11 +318,7 @@ internal static ProcessInfo[] GetProcessInfos(Predicate? processIdFilter = Marshal.FreeHGlobal(unmanaged); } } - - bufferSize = GetNewBufferSize(bufferSize, (int)requiredSize); } - - return processInfos; } private static int GetNewBufferSize(int existingBufferSize, int requiredSize) @@ -333,10 +327,8 @@ private static int GetNewBufferSize(int existingBufferSize, int requiredSize) if (requiredSize == 0) { - // // On some old OS like win2000, requiredSize will not be set if the buffer // passed to NtQuerySystemInformation is not enough. - // newSize = existingBufferSize * 2; } else From d335a91cc463a527df54871b56699ee9c6f8a3da Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 16 Aug 2021 11:18:18 +0200 Subject: [PATCH 10/17] always use ArrayPool --- .../Diagnostics/ProcessManager.Win32.cs | 22 +++---------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index cf544817ac9acc..c303f80f013a4a 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -273,9 +273,6 @@ internal static class NtProcessInfoHelper #else private const int DefaultCachedBufferSize = 1024 * 1024; #endif - // use DefaultCachedBufferSize to ensure we test the unmanaged memory code path path at least in Debug builds - private const int RentSizeLimit = DefaultCachedBufferSize + 1; - private static int MostRecentSize = DefaultCachedBufferSize; internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) @@ -285,18 +282,12 @@ internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) while (true) { - (byte[]? managed, IntPtr unmanaged) = bufferSize < RentSizeLimit - ? (ArrayPool.Shared.Rent(bufferSize), IntPtr.Zero) - : (null, Marshal.AllocHGlobal(bufferSize)); - - Debug.Assert((managed != null && unmanaged == IntPtr.Zero) || (managed == null && unmanaged != IntPtr.Zero)); + byte[] buffer = ArrayPool.Shared.Rent(bufferSize); try { unsafe { - Span buffer = managed != null ? managed : new Span(unmanaged.ToPointer(), bufferSize); - // Note that the buffer will contain pointers to itself and it needs to be pinned while it is being processed // by GetProcessInfos below fixed (byte* bufferPtr = buffer) @@ -325,7 +316,7 @@ internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) MostRecentSize = buffer.Length; // Parse the data block to get process information - return GetProcessInfos(buffer.Slice(firstAlignedElementIndex), processIdFilter); + return GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex), processIdFilter); } bufferSize = GetNewBufferSize(bufferSize, (int)requiredSize); @@ -334,14 +325,7 @@ internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) } finally { - if (managed != null) - { - ArrayPool.Shared.Return(managed); - } - else - { - Marshal.FreeHGlobal(unmanaged); - } + ArrayPool.Shared.Return(buffer); } } } From 4ebeb3d1b1d70cbcf4cba9a4c89a56efb8df191b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 17 Aug 2021 19:32:52 +0200 Subject: [PATCH 11/17] address remaining feedback --- .../src/System/Diagnostics/ProcessManager.Win32.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index c303f80f013a4a..229507f83067d2 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -299,12 +299,12 @@ internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) byte* alignedBufferPtr = (byte*)(((nint)bufferPtr + 7) & ~7); int firstAlignedElementIndex = (int)(alignedBufferPtr - bufferPtr); - uint requiredSize = 0; + uint actualSize = 0; uint status = Interop.NtDll.NtQuerySystemInformation( Interop.NtDll.SystemProcessInformation, alignedBufferPtr, (uint)(buffer.Length - firstAlignedElementIndex), - &requiredSize); + &actualSize); if (status != Interop.NtDll.STATUS_INFO_LENGTH_MISMATCH) { @@ -314,12 +314,13 @@ internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) throw new InvalidOperationException(SR.CouldntGetProcessInfos, new Win32Exception((int)status)); } - MostRecentSize = buffer.Length; + Debug.Assert(actualSize > 0 && actualSize <= bufferSize, $"Actual size reported by NtQuerySystemInformation was {actualSize} for a buffer of size={bufferSize}."); + MostRecentSize = GetNewBufferSize(bufferSize, (int)actualSize); // Parse the data block to get process information return GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex), processIdFilter); } - bufferSize = GetNewBufferSize(bufferSize, (int)requiredSize); + bufferSize = GetNewBufferSize(bufferSize, (int)actualSize); } } } From 48f028e4a34d6393f8ca6bcfef405e21c3c5729c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 17 Aug 2021 20:09:40 +0200 Subject: [PATCH 12/17] Apply suggestions from code review Co-authored-by: Jan Kotas --- .../src/System/Diagnostics/ProcessManager.Win32.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 229507f83067d2..46b599dcff6ae5 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -317,7 +317,7 @@ internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) Debug.Assert(actualSize > 0 && actualSize <= bufferSize, $"Actual size reported by NtQuerySystemInformation was {actualSize} for a buffer of size={bufferSize}."); MostRecentSize = GetNewBufferSize(bufferSize, (int)actualSize); // Parse the data block to get process information - return GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex), processIdFilter); + return GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex, (int)actualSize), processIdFilter); } bufferSize = GetNewBufferSize(bufferSize, (int)actualSize); From fc4378f798edc0f1664334318be493719b46c85b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 17 Aug 2021 20:23:27 +0200 Subject: [PATCH 13/17] remove win2000 workaround --- .../Diagnostics/ProcessManager.Win32.cs | 36 +++++-------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 46b599dcff6ae5..64455670355515 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -298,12 +298,13 @@ internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) Debug.Assert(bufferSize > 8); byte* alignedBufferPtr = (byte*)(((nint)bufferPtr + 7) & ~7); int firstAlignedElementIndex = (int)(alignedBufferPtr - bufferPtr); + bufferSize = buffer.Length - firstAlignedElementIndex; uint actualSize = 0; uint status = Interop.NtDll.NtQuerySystemInformation( Interop.NtDll.SystemProcessInformation, alignedBufferPtr, - (uint)(buffer.Length - firstAlignedElementIndex), + (uint)bufferSize, &actualSize); if (status != Interop.NtDll.STATUS_INFO_LENGTH_MISMATCH) @@ -315,12 +316,13 @@ internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) } Debug.Assert(actualSize > 0 && actualSize <= bufferSize, $"Actual size reported by NtQuerySystemInformation was {actualSize} for a buffer of size={bufferSize}."); - MostRecentSize = GetNewBufferSize(bufferSize, (int)actualSize); + MostRecentSize = GetEstimatedBufferSize(actualSize); // Parse the data block to get process information return GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex, (int)actualSize), processIdFilter); } - bufferSize = GetNewBufferSize(bufferSize, (int)actualSize); + Debug.Assert(actualSize > bufferSize, $"Actual size reported by NtQuerySystemInformation was {actualSize} for a buffer of size={bufferSize}."); + bufferSize = GetEstimatedBufferSize(actualSize); } } } @@ -331,31 +333,9 @@ internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) } } - private static int GetNewBufferSize(int existingBufferSize, int requiredSize) - { - int newSize; - - if (requiredSize == 0) - { - // On some old OS like win2000, requiredSize will not be set if the buffer - // passed to NtQuerySystemInformation is not enough. - newSize = existingBufferSize * 2; - } - else - { - // allocating a few more kilo bytes just in case there are some new process - // kicked in since new call to NtQuerySystemInformation - newSize = requiredSize + 1024 * 10; - } - - if (newSize < 0) - { - // In reality, we should never overflow. - // Adding the code here just in case it happens. - throw new OutOfMemoryException(); - } - return newSize; - } + // allocating a few more kilo bytes just in case there are some new process + // kicked in since new call to NtQuerySystemInformation + private static int GetEstimatedBufferSize(uint actualSize) => (int)actualSize + 1024 * 10; private static unsafe ProcessInfo[] GetProcessInfos(ReadOnlySpan data, int? processIdFilter) { From 1b05dac61d9153905e3eb65cb7df46b409ca05bf Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 18 Aug 2021 21:21:49 +0200 Subject: [PATCH 14/17] use NativeMemory.AlignedAllo --- .../Diagnostics/ProcessManager.Win32.cs | 60 +++++++------------ 1 file changed, 22 insertions(+), 38 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 64455670355515..17e5a7e8111cd3 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -275,60 +275,44 @@ internal static class NtProcessInfoHelper #endif private static int MostRecentSize = DefaultCachedBufferSize; - internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null) + internal static unsafe ProcessInfo[] GetProcessInfos(int? processIdFilter = null) { // Start with the default buffer size. int bufferSize = MostRecentSize; while (true) { - byte[] buffer = ArrayPool.Shared.Rent(bufferSize); + byte* bufferPtr = (byte*)NativeMemory.AlignedAlloc((uint)bufferSize, 8); // some platforms require the buffer to be 64-bit aligned. try { - unsafe + uint actualSize = 0; + uint status = Interop.NtDll.NtQuerySystemInformation( + Interop.NtDll.SystemProcessInformation, + bufferPtr, + (uint)bufferSize, + &actualSize); + + if (status != Interop.NtDll.STATUS_INFO_LENGTH_MISMATCH) { - // Note that the buffer will contain pointers to itself and it needs to be pinned while it is being processed - // by GetProcessInfos below - fixed (byte* bufferPtr = buffer) + // see definition of NT_SUCCESS(Status) in SDK + if ((int)status < 0) { - // some platforms require the buffer to be 64-bit aligned. - // ArrayPool does not guarantee 64-bit alignment, so we take the address of first 64-bit aligned element - // we round up the address of the pinned array to the nearest multiple of 64 - Debug.Assert(bufferSize > 8); - byte* alignedBufferPtr = (byte*)(((nint)bufferPtr + 7) & ~7); - int firstAlignedElementIndex = (int)(alignedBufferPtr - bufferPtr); - bufferSize = buffer.Length - firstAlignedElementIndex; - - uint actualSize = 0; - uint status = Interop.NtDll.NtQuerySystemInformation( - Interop.NtDll.SystemProcessInformation, - alignedBufferPtr, - (uint)bufferSize, - &actualSize); - - if (status != Interop.NtDll.STATUS_INFO_LENGTH_MISMATCH) - { - // see definition of NT_SUCCESS(Status) in SDK - if ((int)status < 0) - { - throw new InvalidOperationException(SR.CouldntGetProcessInfos, new Win32Exception((int)status)); - } - - Debug.Assert(actualSize > 0 && actualSize <= bufferSize, $"Actual size reported by NtQuerySystemInformation was {actualSize} for a buffer of size={bufferSize}."); - MostRecentSize = GetEstimatedBufferSize(actualSize); - // Parse the data block to get process information - return GetProcessInfos(buffer.AsSpan(firstAlignedElementIndex, (int)actualSize), processIdFilter); - } - - Debug.Assert(actualSize > bufferSize, $"Actual size reported by NtQuerySystemInformation was {actualSize} for a buffer of size={bufferSize}."); - bufferSize = GetEstimatedBufferSize(actualSize); + throw new InvalidOperationException(SR.CouldntGetProcessInfos, new Win32Exception((int)status)); } + + Debug.Assert(actualSize > 0 && actualSize <= bufferSize, $"Actual size reported by NtQuerySystemInformation was {actualSize} for a buffer of size={bufferSize}."); + MostRecentSize = GetEstimatedBufferSize(actualSize); + // Parse the data block to get process information + return GetProcessInfos(new ReadOnlySpan(bufferPtr, (int)actualSize), processIdFilter); } + + Debug.Assert(actualSize > bufferSize, $"Actual size reported by NtQuerySystemInformation was {actualSize} for a buffer of size={bufferSize}."); + bufferSize = GetEstimatedBufferSize(actualSize); } finally { - ArrayPool.Shared.Return(buffer); + NativeMemory.AlignedFree(bufferPtr); } } } From 7709e821040fe014aefcb747a94b250b546f966c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 19 Aug 2021 09:10:26 +0200 Subject: [PATCH 15/17] use NativeMemory.Alloc --- .../src/System/Diagnostics/ProcessManager.Win32.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 17e5a7e8111cd3..179d09ed342c26 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -282,7 +282,7 @@ internal static unsafe ProcessInfo[] GetProcessInfos(int? processIdFilter = null while (true) { - byte* bufferPtr = (byte*)NativeMemory.AlignedAlloc((uint)bufferSize, 8); // some platforms require the buffer to be 64-bit aligned. + void* bufferPtr = NativeMemory.Alloc((uint)bufferSize); // some platforms require the buffer to be 64-bit aligned and NativeLibrary.Alloc guarantees sufficient alignment. try { @@ -312,7 +312,7 @@ internal static unsafe ProcessInfo[] GetProcessInfos(int? processIdFilter = null } finally { - NativeMemory.AlignedFree(bufferPtr); + NativeMemory.Free(bufferPtr); } } } From 7872963daa56850f99a7abe6b54c8a8a757ec961 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 19 Aug 2021 09:17:02 +0200 Subject: [PATCH 16/17] use uints everywhere, simplify the code --- .../src/System/Diagnostics/ProcessManager.Win32.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 179d09ed342c26..531951255816eb 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -269,20 +269,20 @@ internal static class NtProcessInfoHelper { // Use a smaller buffer size on debug to ensure we hit the retry path. #if DEBUG - private const int DefaultCachedBufferSize = 8 * 1024; + private const uint DefaultCachedBufferSize = 8 * 1024; #else - private const int DefaultCachedBufferSize = 1024 * 1024; + private const uint DefaultCachedBufferSize = 1024 * 1024; #endif - private static int MostRecentSize = DefaultCachedBufferSize; + private static uint MostRecentSize = DefaultCachedBufferSize; internal static unsafe ProcessInfo[] GetProcessInfos(int? processIdFilter = null) { // Start with the default buffer size. - int bufferSize = MostRecentSize; + uint bufferSize = MostRecentSize; while (true) { - void* bufferPtr = NativeMemory.Alloc((uint)bufferSize); // some platforms require the buffer to be 64-bit aligned and NativeLibrary.Alloc guarantees sufficient alignment. + void* bufferPtr = NativeMemory.Alloc(bufferSize); // some platforms require the buffer to be 64-bit aligned and NativeLibrary.Alloc guarantees sufficient alignment. try { @@ -290,7 +290,7 @@ internal static unsafe ProcessInfo[] GetProcessInfos(int? processIdFilter = null uint status = Interop.NtDll.NtQuerySystemInformation( Interop.NtDll.SystemProcessInformation, bufferPtr, - (uint)bufferSize, + bufferSize, &actualSize); if (status != Interop.NtDll.STATUS_INFO_LENGTH_MISMATCH) @@ -319,7 +319,7 @@ internal static unsafe ProcessInfo[] GetProcessInfos(int? processIdFilter = null // allocating a few more kilo bytes just in case there are some new process // kicked in since new call to NtQuerySystemInformation - private static int GetEstimatedBufferSize(uint actualSize) => (int)actualSize + 1024 * 10; + private static uint GetEstimatedBufferSize(uint actualSize) => actualSize + 1024 * 10; private static unsafe ProcessInfo[] GetProcessInfos(ReadOnlySpan data, int? processIdFilter) { From 7e4de4b9670539e514972cf3aba0d63b11fc5c30 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 19 Aug 2021 16:18:38 +0200 Subject: [PATCH 17/17] apply Stephen suggestion --- .../src/System/Diagnostics/ProcessManager.Win32.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs index 531951255816eb..665bfb455c80e9 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs @@ -268,10 +268,11 @@ private static void HandleLastWin32Error() internal static class NtProcessInfoHelper { // Use a smaller buffer size on debug to ensure we hit the retry path. + private const uint DefaultCachedBufferSize = 1024 * #if DEBUG - private const uint DefaultCachedBufferSize = 8 * 1024; + 8; #else - private const uint DefaultCachedBufferSize = 1024 * 1024; + 1024; #endif private static uint MostRecentSize = DefaultCachedBufferSize;