Merge pull request #3040 from SixLabors/js/memoryallocator-validation

Make UnmanagedMemoryHandle members readonly and improve pool finalization tests

src/ImageSharp/Memory/Allocators/Internals/UnmanagedMemoryHandle.cs

@@ -39,13 +39,13 @@ internal struct UnmanagedMemoryHandle : IEquatable<UnmanagedMemoryHandle>
         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;
 
     /// <summary>
     /// Gets the total outstanding handle allocations for testing purposes.
@@ -121,9 +121,9 @@ internal struct UnmanagedMemoryHandle : IEquatable<UnmanagedMemoryHandle>
         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();
 }

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,75 @@ public class UniformUnmanagedPoolMemoryAllocatorTests
     [InlineData(1200)] // Group of two UniformUnmanagedMemoryPool buffers
     public void AllocateMemoryGroup_Finalization_ReturnsToPool(int length)
     {
-        if (TestEnvironment.IsMacOS)
+        RemoteExecutor.Invoke(RunTest, length.ToString(CultureInfo.InvariantCulture)).Dispose();
-        {
-            // 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();
 
         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<byte>` 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<byte>(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);
+            // Allocate again. If the leaked group was finalized correctly and returned to the pool,
-            GC.Collect();
+            // this should not require additional unmanaged allocations (ie, the handle count must not grow).
-            GC.WaitForPendingFinalizers();
+            allocator.AllocateGroup<byte>(lengthInner, 100).Dispose();
-            GC.Collect();
+
-            GC.WaitForPendingFinalizers();
+            // Note: we use "<=" instead of "==" here.
-
+            //
-            using MemoryGroup<byte> g = allocator.AllocateGroup<byte>(lengthInner, 100);
+            // After we record the baseline, the pool is allowed to legitimately *decrease*
-            Assert.Equal(42, g.First().Span[0]);
+            // `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);
         }
     }
 
-    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<byte> g = allocator.AllocateGroup<byte>(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,
+            // For ArrayPool.Shared, the first rented array may be stored in TLS on the finalizer thread.
-            // repeat rental to make sure per-core buckets are also utilized.
+            // Repeat rental to increase the chance that per-core buckets are involved when length
+            // is small and allocations go through ArrayPool.
             MemoryGroup<byte> g1 = allocator.AllocateGroup<byte>(length, 100);
-            g1.First().Span[0] = 42;
+            g1[0].Span[0] = 42;
+            g1 = null;
         }
+
+        g = null;
     }
 
     [Theory]
@@ -341,69 +350,63 @@ public class UniformUnmanagedPoolMemoryAllocatorTests
     [InlineData(600)] // Group of single UniformUnmanagedMemoryPool buffer
     public void AllocateSingleMemoryOwner_Finalization_ReturnsToPool(int length)
     {
-        if (TestEnvironment.IsMacOS)
+        RemoteExecutor.Invoke(RunTest, length.ToString(CultureInfo.InvariantCulture)).Dispose();
-        {
-            // 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();
 
         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<byte>` 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<byte>(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);
+            // Allocate again. If the leaked owner was finalized correctly and returned to the pool,
-            GC.Collect();
+            // this should not require additional unmanaged allocations (ie, the handle count must not grow).
-            GC.WaitForPendingFinalizers();
+            allocator.Allocate<byte>(lengthInner).Dispose();
-            GC.Collect();
-            GC.WaitForPendingFinalizers();
 
-            using IMemoryOwner<byte> g = allocator.Allocate<byte>(lengthInner);
+            // Note: we use "<=" rather than "==". The pool may legitimately trim and free retained buffers,
-            Assert.Equal(42, g.GetSpan()[0]);
+            // reducing the handle count between baseline and check. The invariant is "no growth".
-            GC.KeepAlive(allocator);
+            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)
     {
+        // Allocate and intentionally do not dispose.
         IMemoryOwner<byte> g = allocator.Allocate<byte>(length);
-        if (check)
-        {
-            Assert.Equal(42, g.GetSpan()[0]);
-        }
 
+        // 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,
+            // For ArrayPool.Shared, the first rented array may be stored in TLS on the finalizer thread.
-            // repeat rental to make sure per-core buckets are also utilized.
+            // Repeat rental to increase the chance that per-core buckets are involved when length
+            // is small and allocations go through ArrayPool.
             IMemoryOwner<byte> g1 = allocator.Allocate<byte>(length);
             g1.GetSpan()[0] = 42;
+            g1 = null;
         }
+
+        g = null;
     }
 
     [Fact]