From 8fda1ef5d0f288fbccce502c06d146ff7a3eae94 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Sat, 8 Feb 2020 00:22:25 +0100 Subject: [PATCH] polish MemoryAllocator API --- ...oolMemoryAllocator.CommonFactoryMethods.cs | 8 ++- .../Allocators/ArrayPoolMemoryAllocator.cs | 41 +++++++++++++--- .../Memory/Allocators/MemoryAllocator.cs | 7 ++- .../Allocators/SimpleGcMemoryAllocator.cs | 2 +- .../InvalidMemoryOperationException.cs | 0 .../ArrayPoolMemoryAllocatorTests.cs | 49 +++++++++++++------ .../BufferExtensions.cs | 4 +- .../BufferTestSuite.cs | 3 +- .../SimpleGcMemoryAllocatorTests.cs | 3 +- 9 files changed, 86 insertions(+), 31 deletions(-) rename src/ImageSharp/Memory/{DiscontiguousBuffers => }/InvalidMemoryOperationException.cs (100%) rename tests/ImageSharp.Tests/Memory/{Alocators => Allocators}/ArrayPoolMemoryAllocatorTests.cs (84%) rename tests/ImageSharp.Tests/Memory/{Alocators => Allocators}/BufferExtensions.cs (93%) rename tests/ImageSharp.Tests/Memory/{Alocators => Allocators}/BufferTestSuite.cs (99%) rename tests/ImageSharp.Tests/Memory/{Alocators => Allocators}/SimpleGcMemoryAllocatorTests.cs (93%) diff --git a/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.CommonFactoryMethods.cs b/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.CommonFactoryMethods.cs index 1ce2525b8..5ef60c9ed 100644 --- a/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.CommonFactoryMethods.cs +++ b/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.CommonFactoryMethods.cs @@ -29,6 +29,9 @@ namespace SixLabors.ImageSharp.Memory /// private const int DefaultNormalPoolBucketCount = 16; + // TODO: This value should be determined by benchmarking + private const int DefaultBufferCapacityInBytes = int.MaxValue / 4; + /// /// This is the default. Should be good for most use cases. /// @@ -39,7 +42,8 @@ namespace SixLabors.ImageSharp.Memory DefaultMaxPooledBufferSizeInBytes, DefaultBufferSelectorThresholdInBytes, DefaultLargePoolBucketCount, - DefaultNormalPoolBucketCount); + DefaultNormalPoolBucketCount, + DefaultBufferCapacityInBytes); } /// @@ -69,4 +73,4 @@ namespace SixLabors.ImageSharp.Memory return new ArrayPoolMemoryAllocator(128 * 1024 * 1024, 32 * 1024 * 1024, 16, 32); } } -} \ No newline at end of file +} diff --git a/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.cs b/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.cs index 883d57851..62f23ba01 100644 --- a/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.cs +++ b/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.cs @@ -60,13 +60,41 @@ namespace SixLabors.ImageSharp.Memory /// The threshold to pool arrays in which has less buckets for memory safety. /// Max arrays per bucket for the large array pool. /// Max arrays per bucket for the normal array pool. - public ArrayPoolMemoryAllocator(int maxPoolSizeInBytes, int poolSelectorThresholdInBytes, int maxArraysPerBucketLargePool, int maxArraysPerBucketNormalPool) + public ArrayPoolMemoryAllocator( + int maxPoolSizeInBytes, + int poolSelectorThresholdInBytes, + int maxArraysPerBucketLargePool, + int maxArraysPerBucketNormalPool) + : this( + maxPoolSizeInBytes, + poolSelectorThresholdInBytes, + maxArraysPerBucketLargePool, + maxArraysPerBucketNormalPool, + DefaultBufferCapacityInBytes) + { + } + + /// + /// Initializes a new instance of the class. + /// + /// The maximum size of pooled arrays. Arrays over the thershold are gonna be always allocated. + /// The threshold to pool arrays in which has less buckets for memory safety. + /// Max arrays per bucket for the large array pool. + /// Max arrays per bucket for the normal array pool. + /// The length of the largest contiguous buffer that can be handled by this allocator instance. + public ArrayPoolMemoryAllocator( + int maxPoolSizeInBytes, + int poolSelectorThresholdInBytes, + int maxArraysPerBucketLargePool, + int maxArraysPerBucketNormalPool, + int bufferCapacityInBytes) { Guard.MustBeGreaterThan(maxPoolSizeInBytes, 0, nameof(maxPoolSizeInBytes)); Guard.MustBeLessThanOrEqualTo(poolSelectorThresholdInBytes, maxPoolSizeInBytes, nameof(poolSelectorThresholdInBytes)); this.MaxPoolSizeInBytes = maxPoolSizeInBytes; this.PoolSelectorThresholdInBytes = poolSelectorThresholdInBytes; + this.BufferCapacityInBytes = bufferCapacityInBytes; this.maxArraysPerBucketLargePool = maxArraysPerBucketLargePool; this.maxArraysPerBucketNormalPool = maxArraysPerBucketNormalPool; @@ -84,9 +112,9 @@ namespace SixLabors.ImageSharp.Memory public int PoolSelectorThresholdInBytes { get; } /// - /// Gets or sets the length of the largest contiguous buffer that can be handled by this allocator instance. + /// Gets the length of the largest contiguous buffer that can be handled by this allocator instance. /// - public int BufferCapacityInBytes { get; set; } = DefaultBufferCapacity; + public int BufferCapacityInBytes { get; internal set; } // Setter is internal for easy configuration in tests /// public override void ReleaseRetainedResources() @@ -103,11 +131,10 @@ namespace SixLabors.ImageSharp.Memory Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length)); int itemSizeBytes = Unsafe.SizeOf(); int bufferSizeInBytes = length * itemSizeBytes; - if (bufferSizeInBytes < 0) + if (bufferSizeInBytes < 0 || bufferSizeInBytes > BufferCapacityInBytes) { - throw new ArgumentOutOfRangeException( - nameof(length), - $"{nameof(ArrayPoolMemoryAllocator)} can not allocate {length} elements of {typeof(T).Name}."); + throw new InvalidMemoryOperationException( + $"Requested allocation {length} elements of {typeof(T).Name} is over the capacity of the MemoryAllocator."); } ArrayPool pool = this.GetArrayPool(bufferSizeInBytes); diff --git a/src/ImageSharp/Memory/Allocators/MemoryAllocator.cs b/src/ImageSharp/Memory/Allocators/MemoryAllocator.cs index c6e92f23f..d02d50d9d 100644 --- a/src/ImageSharp/Memory/Allocators/MemoryAllocator.cs +++ b/src/ImageSharp/Memory/Allocators/MemoryAllocator.cs @@ -1,6 +1,7 @@ // Copyright (c) Six Labors and contributors. // Licensed under the Apache License, Version 2.0. +using System; using System.Buffers; namespace SixLabors.ImageSharp.Memory @@ -10,7 +11,7 @@ namespace SixLabors.ImageSharp.Memory /// public abstract class MemoryAllocator { - internal const int DefaultBufferCapacity = int.MaxValue / 2; + /// /// Gets the length of the largest contiguous buffer that can be handled by this allocator instance in bytes. @@ -25,6 +26,8 @@ namespace SixLabors.ImageSharp.Memory /// Size of the buffer to allocate. /// The allocation options. /// A buffer of values of type . + /// When length is zero or negative. + /// When length is over the capacity of the allocator. public abstract IMemoryOwner Allocate(int length, AllocationOptions options = AllocationOptions.None) where T : struct; @@ -34,6 +37,8 @@ namespace SixLabors.ImageSharp.Memory /// The requested buffer length. /// The allocation options. /// The . + /// When length is zero or negative. + /// When length is over the capacity of the allocator. public abstract IManagedByteBuffer AllocateManagedByteBuffer(int length, AllocationOptions options = AllocationOptions.None); /// diff --git a/src/ImageSharp/Memory/Allocators/SimpleGcMemoryAllocator.cs b/src/ImageSharp/Memory/Allocators/SimpleGcMemoryAllocator.cs index b417df351..4c62e4ded 100644 --- a/src/ImageSharp/Memory/Allocators/SimpleGcMemoryAllocator.cs +++ b/src/ImageSharp/Memory/Allocators/SimpleGcMemoryAllocator.cs @@ -12,7 +12,7 @@ namespace SixLabors.ImageSharp.Memory public sealed class SimpleGcMemoryAllocator : MemoryAllocator { /// - protected internal override int GetBufferCapacityInBytes() => DefaultBufferCapacity; + protected internal override int GetBufferCapacityInBytes() => int.MaxValue; /// public override IMemoryOwner Allocate(int length, AllocationOptions options = AllocationOptions.None) diff --git a/src/ImageSharp/Memory/DiscontiguousBuffers/InvalidMemoryOperationException.cs b/src/ImageSharp/Memory/InvalidMemoryOperationException.cs similarity index 100% rename from src/ImageSharp/Memory/DiscontiguousBuffers/InvalidMemoryOperationException.cs rename to src/ImageSharp/Memory/InvalidMemoryOperationException.cs diff --git a/tests/ImageSharp.Tests/Memory/Alocators/ArrayPoolMemoryAllocatorTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/ArrayPoolMemoryAllocatorTests.cs similarity index 84% rename from tests/ImageSharp.Tests/Memory/Alocators/ArrayPoolMemoryAllocatorTests.cs rename to tests/ImageSharp.Tests/Memory/Allocators/ArrayPoolMemoryAllocatorTests.cs index dd497e57e..1e079fcf5 100644 --- a/tests/ImageSharp.Tests/Memory/Alocators/ArrayPoolMemoryAllocatorTests.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/ArrayPoolMemoryAllocatorTests.cs @@ -1,18 +1,15 @@ // Copyright (c) Six Labors and contributors. // Licensed under the Apache License, Version 2.0. -// ReSharper disable InconsistentNaming using System; using System.Buffers; -using System.Diagnostics; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using Microsoft.DotNet.RemoteExecutor; -using Microsoft.Win32; -using SixLabors.ImageSharp.Tests; +using SixLabors.ImageSharp.Memory; using Xunit; -namespace SixLabors.ImageSharp.Memory.Tests +namespace SixLabors.ImageSharp.Tests.Memory.Allocators { public class ArrayPoolMemoryAllocatorTests { @@ -116,13 +113,13 @@ namespace SixLabors.ImageSharp.Memory.Tests MemoryAllocator memoryAllocator = this.LocalFixture.MemoryAllocator; using (IMemoryOwner firstAlloc = memoryAllocator.Allocate(42)) { - firstAlloc.GetSpan().Fill(666); + BufferExtensions.GetSpan(firstAlloc).Fill(666); } using (IMemoryOwner secondAlloc = memoryAllocator.Allocate(42, options)) { int expected = options == AllocationOptions.Clean ? 0 : 666; - Assert.Equal(expected, secondAlloc.GetSpan()[0]); + Assert.Equal(expected, BufferExtensions.GetSpan(secondAlloc)[0]); } } @@ -133,7 +130,7 @@ namespace SixLabors.ImageSharp.Memory.Tests { MemoryAllocator memoryAllocator = this.LocalFixture.MemoryAllocator; IMemoryOwner buffer = memoryAllocator.Allocate(32); - ref int ptrToPrev0 = ref MemoryMarshal.GetReference(buffer.GetSpan()); + ref int ptrToPrev0 = ref MemoryMarshal.GetReference(BufferExtensions.GetSpan(buffer)); if (!keepBufferAlive) { @@ -144,7 +141,7 @@ namespace SixLabors.ImageSharp.Memory.Tests buffer = memoryAllocator.Allocate(32); - Assert.False(Unsafe.AreSame(ref ptrToPrev0, ref buffer.GetReference())); + Assert.False(Unsafe.AreSame(ref ptrToPrev0, ref BufferExtensions.GetReference(buffer))); } [Fact] @@ -164,12 +161,12 @@ namespace SixLabors.ImageSharp.Memory.Tests const int ArrayLengthThreshold = PoolSelectorThresholdInBytes / sizeof(int); IMemoryOwner small = StaticFixture.MemoryAllocator.Allocate(ArrayLengthThreshold - 1); - ref int ptr2Small = ref small.GetReference(); + ref int ptr2Small = ref BufferExtensions.GetReference(small); small.Dispose(); IMemoryOwner large = StaticFixture.MemoryAllocator.Allocate(ArrayLengthThreshold + 1); - Assert.False(Unsafe.AreSame(ref ptr2Small, ref large.GetReference())); + Assert.False(Unsafe.AreSame(ref ptr2Small, ref BufferExtensions.GetReference(large))); } RemoteExecutor.Invoke(RunTest).Dispose(); @@ -216,14 +213,34 @@ namespace SixLabors.ImageSharp.Memory.Tests [Theory] [InlineData(-1)] - [InlineData((int.MaxValue / SizeOfLargeStruct) + 1)] - public void AllocateIncorrectAmount_ThrowsCorrect_ArgumentOutOfRangeException(int length) + [InlineData(-111)] + public void Allocate_Negative_Throws_ArgumentOutOfRangeException(int length) { ArgumentOutOfRangeException ex = Assert.Throws(() => this.LocalFixture.MemoryAllocator.Allocate(length)); Assert.Equal("length", ex.ParamName); } + [Fact] + public void AllocateZero() + { + using IMemoryOwner buffer = this.LocalFixture.MemoryAllocator.Allocate(0); + Assert.Equal(0, buffer.Memory.Length); + } + + [Theory] + [InlineData(101)] + [InlineData((int.MaxValue / SizeOfLargeStruct) - 1)] + [InlineData(int.MaxValue / SizeOfLargeStruct)] + [InlineData((int.MaxValue / SizeOfLargeStruct) + 1)] + [InlineData((int.MaxValue / SizeOfLargeStruct) + 137)] + public void Allocate_OverCapacity_Throws_InvalidMemoryOperationException(int length) + { + this.LocalFixture.MemoryAllocator.BufferCapacityInBytes = 100 * SizeOfLargeStruct; + Assert.Throws(() => + this.LocalFixture.MemoryAllocator.Allocate(length)); + } + [Theory] [InlineData(-1)] public void AllocateManagedByteBuffer_IncorrectAmount_ThrowsCorrect_ArgumentOutOfRangeException(int length) @@ -235,7 +252,7 @@ namespace SixLabors.ImageSharp.Memory.Tests private class MemoryAllocatorFixture { - public MemoryAllocator MemoryAllocator { get; set; } = + public ArrayPoolMemoryAllocator MemoryAllocator { get; set; } = new ArrayPoolMemoryAllocator(MaxPooledBufferSizeInBytes, PoolSelectorThresholdInBytes); /// @@ -245,11 +262,11 @@ namespace SixLabors.ImageSharp.Memory.Tests where T : struct { IMemoryOwner buffer = this.MemoryAllocator.Allocate(length); - ref T ptrToPrevPosition0 = ref buffer.GetReference(); + ref T ptrToPrevPosition0 = ref BufferExtensions.GetReference(buffer); buffer.Dispose(); buffer = this.MemoryAllocator.Allocate(length); - bool sameBuffers = Unsafe.AreSame(ref ptrToPrevPosition0, ref buffer.GetReference()); + bool sameBuffers = Unsafe.AreSame(ref ptrToPrevPosition0, ref BufferExtensions.GetReference(buffer)); buffer.Dispose(); return sameBuffers; diff --git a/tests/ImageSharp.Tests/Memory/Alocators/BufferExtensions.cs b/tests/ImageSharp.Tests/Memory/Allocators/BufferExtensions.cs similarity index 93% rename from tests/ImageSharp.Tests/Memory/Alocators/BufferExtensions.cs rename to tests/ImageSharp.Tests/Memory/Allocators/BufferExtensions.cs index 8073d069d..9f8543fff 100644 --- a/tests/ImageSharp.Tests/Memory/Alocators/BufferExtensions.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/BufferExtensions.cs @@ -6,7 +6,7 @@ using System.Buffers; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; -namespace SixLabors.ImageSharp.Memory.Tests +namespace SixLabors.ImageSharp.Tests.Memory.Allocators { internal static class BufferExtensions { @@ -22,4 +22,4 @@ namespace SixLabors.ImageSharp.Memory.Tests where T : struct => ref MemoryMarshal.GetReference(buffer.GetSpan()); } -} \ No newline at end of file +} diff --git a/tests/ImageSharp.Tests/Memory/Alocators/BufferTestSuite.cs b/tests/ImageSharp.Tests/Memory/Allocators/BufferTestSuite.cs similarity index 99% rename from tests/ImageSharp.Tests/Memory/Alocators/BufferTestSuite.cs rename to tests/ImageSharp.Tests/Memory/Allocators/BufferTestSuite.cs index 4590bbe97..6465e0b81 100644 --- a/tests/ImageSharp.Tests/Memory/Alocators/BufferTestSuite.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/BufferTestSuite.cs @@ -5,10 +5,11 @@ using System; using System.Buffers; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +using SixLabors.ImageSharp.Memory; using Xunit; // ReSharper disable InconsistentNaming -namespace SixLabors.ImageSharp.Memory.Tests +namespace SixLabors.ImageSharp.Tests.Memory.Allocators { /// /// Inherit this class to test an implementation (provided by ). diff --git a/tests/ImageSharp.Tests/Memory/Alocators/SimpleGcMemoryAllocatorTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/SimpleGcMemoryAllocatorTests.cs similarity index 93% rename from tests/ImageSharp.Tests/Memory/Alocators/SimpleGcMemoryAllocatorTests.cs rename to tests/ImageSharp.Tests/Memory/Allocators/SimpleGcMemoryAllocatorTests.cs index 8e3b82be5..9e14bd1db 100644 --- a/tests/ImageSharp.Tests/Memory/Alocators/SimpleGcMemoryAllocatorTests.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/SimpleGcMemoryAllocatorTests.cs @@ -3,9 +3,10 @@ using System; using System.Runtime.InteropServices; +using SixLabors.ImageSharp.Memory; using Xunit; -namespace SixLabors.ImageSharp.Memory.Tests +namespace SixLabors.ImageSharp.Tests.Memory.Allocators { public class SimpleGcMemoryAllocatorTests {