From afc97dfbe6a9a36ccb4b155e45a87f4574af993e Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Tue, 12 May 2026 19:43:20 +1000 Subject: [PATCH] Address Copilot feedback --- .../AllocationTrackedMemoryManager{T}.cs | 10 +++- .../Memory/Allocators/MemoryAllocator.cs | 60 ++++++++++++------- .../Allocators/UnmanagedMemoryAllocator.cs | 4 -- .../Memory/InvalidMemoryOperationException.cs | 5 ++ .../Image/ProcessPixelRowsTestBase.cs | 12 +++- 5 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/ImageSharp/Memory/AllocationTrackedMemoryManager{T}.cs b/src/ImageSharp/Memory/AllocationTrackedMemoryManager{T}.cs index 36e116297a..b2e34f3dd3 100644 --- a/src/ImageSharp/Memory/AllocationTrackedMemoryManager{T}.cs +++ b/src/ImageSharp/Memory/AllocationTrackedMemoryManager{T}.cs @@ -35,8 +35,14 @@ public abstract class AllocationTrackedMemoryManager : MemoryManager /// protected sealed override void Dispose(bool disposing) { - this.DisposeCore(disposing); - this.ReleaseAllocationTracking(); + try + { + this.DisposeCore(disposing); + } + finally + { + this.ReleaseAllocationTracking(); + } } /// diff --git a/src/ImageSharp/Memory/Allocators/MemoryAllocator.cs b/src/ImageSharp/Memory/Allocators/MemoryAllocator.cs index b1dc40e6b3..7a694aea26 100644 --- a/src/ImageSharp/Memory/Allocators/MemoryAllocator.cs +++ b/src/ImageSharp/Memory/Allocators/MemoryAllocator.cs @@ -102,27 +102,15 @@ public abstract class MemoryAllocator /// Size of the buffer to allocate. /// The allocation options. /// A buffer of values of type . - /// When length is negative. - /// When length is over the capacity of the allocator. + /// When length is negative or over the capacity of the allocator. public IMemoryOwner Allocate(int length, AllocationOptions options = AllocationOptions.None) where T : struct { - if (length < 0) - { - InvalidMemoryOperationException.ThrowNegativeAllocationException(length); - } - - ulong lengthInBytes = (ulong)length * (ulong)Unsafe.SizeOf(); - if (lengthInBytes > (ulong)this.SingleBufferAllocationLimitBytes) - { - InvalidMemoryOperationException.ThrowAllocationOverLimitException(lengthInBytes, this.SingleBufferAllocationLimitBytes); - } - - long lengthInBytesLong = (long)lengthInBytes; - bool shouldTrack = lengthInBytesLong != 0; + long lengthInBytes = this.GetValidatedAllocationLengthInBytes(length); + bool shouldTrack = this.AccumulativeAllocationLimitBytes != long.MaxValue && lengthInBytes != 0; if (shouldTrack) { - this.ReserveAllocation(lengthInBytesLong); + this.ReserveAllocation(lengthInBytes); } try @@ -130,7 +118,7 @@ public abstract class MemoryAllocator AllocationTrackedMemoryManager owner = this.AllocateCore(length, options); if (shouldTrack) { - owner.AttachAllocationTracking(this, lengthInBytesLong); + owner.AttachAllocationTracking(this, lengthInBytes); } return owner; @@ -139,7 +127,7 @@ public abstract class MemoryAllocator { if (shouldTrack) { - this.ReleaseAccumulatedBytes(lengthInBytesLong); + this.ReleaseAccumulatedBytes(lengthInBytes); } throw; @@ -199,7 +187,7 @@ public abstract class MemoryAllocator } long totalLengthInBytesLong = (long)totalLengthInBytes; - bool shouldTrack = totalLengthInBytesLong != 0; + bool shouldTrack = this.AccumulativeAllocationLimitBytes != long.MaxValue && totalLengthInBytesLong != 0; if (shouldTrack) { this.ReserveAllocation(totalLengthInBytesLong); @@ -238,12 +226,38 @@ public abstract class MemoryAllocator /// The allocation options. /// A segment owner for the requested buffer length. /// - /// The default implementation uses . Built-in allocators - /// can override this to supply raw segment owners when group construction must bypass nested tracking. + /// The default implementation validates the segment size then calls + /// directly so group construction can reserve and release the total allocation once. /// internal virtual IMemoryOwner AllocateGroupBuffer(int length, AllocationOptions options = AllocationOptions.None) where T : struct - => this.AllocateCore(length, options); + { + _ = this.GetValidatedAllocationLengthInBytes(length); + return this.AllocateCore(length, options); + } + + /// + /// Returns the validated allocation length in bytes. + /// + /// Type of the data stored in the buffer. + /// Size of the buffer to allocate. + /// The allocation length in bytes. + private long GetValidatedAllocationLengthInBytes(int length) + where T : struct + { + if (length < 0) + { + InvalidMemoryOperationException.ThrowNegativeAllocationException(length); + } + + ulong lengthInBytes = (ulong)length * (ulong)Unsafe.SizeOf(); + if (lengthInBytes > (ulong)this.SingleBufferAllocationLimitBytes) + { + InvalidMemoryOperationException.ThrowAllocationOverLimitException(lengthInBytes, this.SingleBufferAllocationLimitBytes); + } + + return (long)lengthInBytes; + } /// /// Reserves accumulative allocation bytes before creating the underlying buffer. @@ -260,7 +274,7 @@ public abstract class MemoryAllocator if (total > this.AccumulativeAllocationLimitBytes) { _ = Interlocked.Add(ref this.accumulativeAllocatedBytes, -lengthInBytes); - InvalidMemoryOperationException.ThrowAllocationOverLimitException((ulong)lengthInBytes, this.AccumulativeAllocationLimitBytes); + InvalidMemoryOperationException.ThrowAccumulativeAllocationOverLimitException(lengthInBytes, total, this.AccumulativeAllocationLimitBytes); } } diff --git a/src/ImageSharp/Memory/Allocators/UnmanagedMemoryAllocator.cs b/src/ImageSharp/Memory/Allocators/UnmanagedMemoryAllocator.cs index 69a4af70ed..eb52da7c03 100644 --- a/src/ImageSharp/Memory/Allocators/UnmanagedMemoryAllocator.cs +++ b/src/ImageSharp/Memory/Allocators/UnmanagedMemoryAllocator.cs @@ -22,10 +22,6 @@ internal class UnmanagedMemoryAllocator : MemoryAllocator where T : struct => AllocateBuffer(length, options); - internal override IMemoryOwner AllocateGroupBuffer(int length, AllocationOptions options = AllocationOptions.None) - where T : struct - => AllocateBuffer(length, options); - // The pooled allocator uses this internal entry point when it needs a raw unmanaged owner without // nesting another allocator-level reservation cycle around the fallback allocation. internal static UnmanagedBuffer AllocateBuffer(int length, AllocationOptions options = AllocationOptions.None) diff --git a/src/ImageSharp/Memory/InvalidMemoryOperationException.cs b/src/ImageSharp/Memory/InvalidMemoryOperationException.cs index 81210f13db..2722747a1b 100644 --- a/src/ImageSharp/Memory/InvalidMemoryOperationException.cs +++ b/src/ImageSharp/Memory/InvalidMemoryOperationException.cs @@ -39,4 +39,9 @@ public class InvalidMemoryOperationException : InvalidOperationException [DoesNotReturn] internal static void ThrowAllocationOverLimitException(ulong length, long limit) => throw new InvalidMemoryOperationException($"Attempted to allocate a buffer of length={length} that exceeded the limit {limit}."); + + [DoesNotReturn] + internal static void ThrowAccumulativeAllocationOverLimitException(long requestedLength, long totalLength, long limit) => + throw new InvalidMemoryOperationException( + $"Attempted to allocate a buffer of length={requestedLength} that would increase the cumulative allocation size to {totalLength}, exceeding the limit {limit}."); } diff --git a/tests/ImageSharp.Tests/Image/ProcessPixelRowsTestBase.cs b/tests/ImageSharp.Tests/Image/ProcessPixelRowsTestBase.cs index cbd6a9e147..a5dfb0d23f 100644 --- a/tests/ImageSharp.Tests/Image/ProcessPixelRowsTestBase.cs +++ b/tests/ImageSharp.Tests/Image/ProcessPixelRowsTestBase.cs @@ -313,7 +313,15 @@ public abstract class ProcessPixelRowsTestBase protected internal override int GetBufferCapacityInBytes() => int.MaxValue; - protected override AllocationTrackedMemoryManager AllocateCore(int length, AllocationOptions options = AllocationOptions.None) => - this.buffers.Pop() as AllocationTrackedMemoryManager; + protected override AllocationTrackedMemoryManager AllocateCore(int length, AllocationOptions options = AllocationOptions.None) + { + object buffer = this.buffers.Pop(); + if (buffer is AllocationTrackedMemoryManager trackedBuffer) + { + return trackedBuffer; + } + + throw new InvalidMemoryOperationException("The requested buffer type does not match the mock allocator buffer type."); + } } }