From bca619751783680b416b140ca0b93991f197f166 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 12 Jan 2026 23:18:55 +1000 Subject: [PATCH 1/2] Make UnmanagedMemoryHandle members readonly and improve pool finalization tests --- .../Internals/UnmanagedMemoryHandle.cs | 14 +- ...niformUnmanagedPoolMemoryAllocatorTests.cs | 162 +++++++++--------- 2 files changed, 90 insertions(+), 86 deletions(-) diff --git a/src/ImageSharp/Memory/Allocators/Internals/UnmanagedMemoryHandle.cs b/src/ImageSharp/Memory/Allocators/Internals/UnmanagedMemoryHandle.cs index 6b31cadf4f..632e1bec04 100644 --- a/src/ImageSharp/Memory/Allocators/Internals/UnmanagedMemoryHandle.cs +++ b/src/ImageSharp/Memory/Allocators/Internals/UnmanagedMemoryHandle.cs @@ -39,13 +39,13 @@ private UnmanagedMemoryHandle(IntPtr handle, int lengthInBytes) Interlocked.Increment(ref totalOutstandingHandles); } - public IntPtr Handle => this.handle; + public readonly IntPtr Handle => this.handle; - public bool IsInvalid => this.Handle == IntPtr.Zero; + public readonly bool IsInvalid => this.Handle == IntPtr.Zero; - public bool IsValid => this.Handle != IntPtr.Zero; + public readonly bool IsValid => this.Handle != IntPtr.Zero; - public unsafe void* Pointer => (void*)this.Handle; + public readonly unsafe void* Pointer => (void*)this.Handle; /// /// Gets the total outstanding handle allocations for testing purposes. @@ -121,9 +121,9 @@ public void Free() this.lengthInBytes = 0; } - public bool Equals(UnmanagedMemoryHandle other) => this.handle.Equals(other.handle); + public readonly bool Equals(UnmanagedMemoryHandle other) => this.handle.Equals(other.handle); - public override bool Equals(object? obj) => obj is UnmanagedMemoryHandle other && this.Equals(other); + public override readonly bool Equals(object? obj) => obj is UnmanagedMemoryHandle other && this.Equals(other); - public override int GetHashCode() => this.handle.GetHashCode(); + public override readonly int GetHashCode() => this.handle.GetHashCode(); } diff --git a/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs index 6e7e9fea73..f3cb6a5e56 100644 --- a/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs @@ -2,6 +2,7 @@ // Licensed under the Six Labors Split License. using System.Buffers; +using System.Globalization; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using Microsoft.DotNet.RemoteExecutor; @@ -273,67 +274,76 @@ static void RunTest() [InlineData(1200)] // Group of two UniformUnmanagedMemoryPool buffers public void AllocateMemoryGroup_Finalization_ReturnsToPool(int length) { - if (TestEnvironment.IsMacOS) - { - // Skip on macOS: https://github.com/SixLabors/ImageSharp/issues/1887 - return; - } - - if (TestEnvironment.OSArchitecture == Architecture.Arm64) - { - // Skip on ARM64: https://github.com/SixLabors/ImageSharp/issues/2342 - return; - } - - if (!TestEnvironment.RunsOnCI) - { - // This may fail in local runs resulting in high memory load. - // Remove the condition for local debugging! - return; - } - - // RunTest(length.ToString()); - RemoteExecutor.Invoke(RunTest, length.ToString()).Dispose(); + RemoteExecutor.Invoke(RunTest, length.ToString(CultureInfo.InvariantCulture)).Dispose(); static void RunTest(string lengthStr) { UniformUnmanagedMemoryPoolMemoryAllocator allocator = new(512, 1024, 16 * 1024, 1024); - int lengthInner = int.Parse(lengthStr); - + int lengthInner = int.Parse(lengthStr, CultureInfo.InvariantCulture); + + // We want to verify that a leaked (not disposed) `MemoryGroup` still returns its + // unmanaged handles into the pool when it is finalized. + // + // We intentionally do NOT validate this by checking the contents of the re-rented memory + // (contents are not guaranteed to be preserved) nor by comparing pointer values + // (the pool may return a different handle while still correctly pooling). + // + // Instead, we validate that after a forced GC+finalization cycle, a subsequent allocation + // of the same size does not cause the number of outstanding unmanaged handles to *increase* + // compared to a known baseline. + + // Establish a baseline: create one allocation and dispose it so the pool is initialized. + // (This ensures subsequent observations are not biased by first-time pool growth.) + allocator.AllocateGroup(lengthInner, 100).Dispose(); + int baselineHandles = UnmanagedMemoryHandle.TotalOutstandingHandles; + + // Leak one allocation and force finalization. AllocateGroupAndForget(allocator, lengthInner); GC.Collect(); GC.WaitForPendingFinalizers(); GC.Collect(); GC.WaitForPendingFinalizers(); - AllocateGroupAndForget(allocator, lengthInner, true); - GC.Collect(); - GC.WaitForPendingFinalizers(); - GC.Collect(); - GC.WaitForPendingFinalizers(); - - using MemoryGroup g = allocator.AllocateGroup(lengthInner, 100); - Assert.Equal(42, g.First().Span[0]); + // Allocate again. If the leaked group was finalized correctly and returned to the pool, + // this should not require additional unmanaged allocations (ie, the handle count must not grow). + allocator.AllocateGroup(lengthInner, 100).Dispose(); + + // Note: we use "<=" instead of "==" here. + // + // After we record the baseline, the pool is allowed to legitimately *decrease* + // `UnmanagedMemoryHandle.TotalOutstandingHandles` by trimming retained buffers + // (eg. via the pool's trim timer/GC callbacks/high-pressure logic). + // + // What must not happen is the opposite: the leaked (non-disposed) group should be finalized + // and its handles returned to the pool such that allocating again does NOT require creating + // additional unmanaged handles. Therefore the only invariant we can reliably assert here is + // "no growth" relative to the baseline. + Assert.True(UnmanagedMemoryHandle.TotalOutstandingHandles <= baselineHandles); + GC.KeepAlive(allocator); } } - private static void AllocateGroupAndForget(UniformUnmanagedMemoryPoolMemoryAllocator allocator, int length, bool check = false) + [MethodImpl(MethodImplOptions.NoInlining)] + private static void AllocateGroupAndForget(UniformUnmanagedMemoryPoolMemoryAllocator allocator, int length) { + // Allocate a group and drop the reference without disposing. + // The test relies on the group's finalizer to return the rented memory to the pool. MemoryGroup g = allocator.AllocateGroup(length, 100); - if (check) - { - Assert.Equal(42, g.First().Span[0]); - } - g.First().Span[0] = 42; + // Touch the memory to ensure the buffer is actually materialized/usable. + g[0].Span[0] = 42; if (length < 512) { - // For ArrayPool.Shared, first array will be returned to the TLS storage of the finalizer thread, - // repeat rental to make sure per-core buckets are also utilized. + // For ArrayPool.Shared, the first rented array may be stored in TLS on the finalizer thread. + // Repeat rental to increase the chance that per-core buckets are involved when length + // is small and allocations go through ArrayPool. MemoryGroup g1 = allocator.AllocateGroup(length, 100); - g1.First().Span[0] = 42; + g1[0].Span[0] = 42; + g1 = null; } + + g = null; } [Theory] @@ -341,69 +351,63 @@ private static void AllocateGroupAndForget(UniformUnmanagedMemoryPoolMemoryAlloc [InlineData(600)] // Group of single UniformUnmanagedMemoryPool buffer public void AllocateSingleMemoryOwner_Finalization_ReturnsToPool(int length) { - if (TestEnvironment.IsMacOS) - { - // Skip on macOS: https://github.com/SixLabors/ImageSharp/issues/1887 - return; - } - - if (TestEnvironment.OSArchitecture == Architecture.Arm64) - { - // Skip on ARM64: https://github.com/SixLabors/ImageSharp/issues/2342 - return; - } - - if (!TestEnvironment.RunsOnCI) - { - // This may fail in local runs resulting in high memory load. - // Remove the condition for local debugging! - return; - } - - // RunTest(length.ToString()); - RemoteExecutor.Invoke(RunTest, length.ToString()).Dispose(); + RemoteExecutor.Invoke(RunTest, length.ToString(CultureInfo.InvariantCulture)).Dispose(); static void RunTest(string lengthStr) { UniformUnmanagedMemoryPoolMemoryAllocator allocator = new(512, 1024, 16 * 1024, 1024); - int lengthInner = int.Parse(lengthStr); - + int lengthInner = int.Parse(lengthStr, CultureInfo.InvariantCulture); + + // This test verifies pooling behavior when an `IMemoryOwner` is leaked (not disposed) + // and must be returned to the pool by finalization. + // + // We do NOT use a sentinel byte value to prove reuse because the contents of pooled buffers + // are not required to be preserved across rentals. + // + // Instead, we assert that after forcing GC+finalization, renting the same size again does not + // increase `UnmanagedMemoryHandle.TotalOutstandingHandles` above a baseline. + + // Establish a baseline: allocate+dispose once so the pool has a chance to materialize/retain buffers. + allocator.Allocate(lengthInner).Dispose(); + int baselineHandles = UnmanagedMemoryHandle.TotalOutstandingHandles; + + // Leak one allocation and force finalization. AllocateSingleAndForget(allocator, lengthInner); GC.Collect(); GC.WaitForPendingFinalizers(); GC.Collect(); GC.WaitForPendingFinalizers(); - AllocateSingleAndForget(allocator, lengthInner, true); - GC.Collect(); - GC.WaitForPendingFinalizers(); - GC.Collect(); - GC.WaitForPendingFinalizers(); + // Allocate again. If the leaked owner was finalized correctly and returned to the pool, + // this should not require additional unmanaged allocations (ie, the handle count must not grow). + allocator.Allocate(lengthInner).Dispose(); - using IMemoryOwner g = allocator.Allocate(lengthInner); - Assert.Equal(42, g.GetSpan()[0]); - GC.KeepAlive(allocator); + // Note: we use "<=" rather than "==". The pool may legitimately trim and free retained buffers, + // reducing the handle count between baseline and check. The invariant is "no growth". + Assert.True(UnmanagedMemoryHandle.TotalOutstandingHandles <= baselineHandles); } } [MethodImpl(MethodImplOptions.NoInlining)] - private static void AllocateSingleAndForget(UniformUnmanagedMemoryPoolMemoryAllocator allocator, int length, bool check = false) + private static void AllocateSingleAndForget(UniformUnmanagedMemoryPoolMemoryAllocator allocator, int length) { - IMemoryOwner g = allocator.Allocate(length); - if (check) - { - Assert.Equal(42, g.GetSpan()[0]); - } + // Allocate and intentionally do not dispose. + IMemoryOwner? g = allocator.Allocate(length); + // Touch the memory to ensure the buffer is actually materialized/usable. g.GetSpan()[0] = 42; if (length < 512) { - // For ArrayPool.Shared, first array will be returned to the TLS storage of the finalizer thread, - // repeat rental to make sure per-core buckets are also utilized. + // For ArrayPool.Shared, the first rented array may be stored in TLS on the finalizer thread. + // Repeat rental to increase the chance that per-core buckets are involved when length + // is small and allocations go through ArrayPool. IMemoryOwner g1 = allocator.Allocate(length); g1.GetSpan()[0] = 42; + g1 = null; } + + g = null; } [Fact] From 55818d39d6fd2ff3baf0adf418131adea29c4d73 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 12 Jan 2026 23:31:42 +1000 Subject: [PATCH 2/2] Minor cleanup --- .../Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs index f3cb6a5e56..c1f5b44bf7 100644 --- a/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs @@ -319,7 +319,6 @@ static void RunTest(string lengthStr) // additional unmanaged handles. Therefore the only invariant we can reliably assert here is // "no growth" relative to the baseline. Assert.True(UnmanagedMemoryHandle.TotalOutstandingHandles <= baselineHandles); - GC.KeepAlive(allocator); } } @@ -392,7 +391,7 @@ static void RunTest(string lengthStr) private static void AllocateSingleAndForget(UniformUnmanagedMemoryPoolMemoryAllocator allocator, int length) { // Allocate and intentionally do not dispose. - IMemoryOwner? g = allocator.Allocate(length); + IMemoryOwner g = allocator.Allocate(length); // Touch the memory to ensure the buffer is actually materialized/usable. g.GetSpan()[0] = 42;