-
-
Notifications
You must be signed in to change notification settings - Fork 855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Limit all memory allocations in the MemoryAllocator layer #2706
Changes from 2 commits
92b8277
7dd3c43
63d4b20
eec9718
da49788
d1cc651
572366e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ | |
// Licensed under the Six Labors Split License. | ||
|
||
using System.Buffers; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Runtime.CompilerServices; | ||
|
||
namespace SixLabors.ImageSharp.Memory; | ||
|
||
|
@@ -10,6 +12,8 @@ namespace SixLabors.ImageSharp.Memory; | |
/// </summary> | ||
public abstract class MemoryAllocator | ||
{ | ||
private const int OneGigabyte = 1 << 30; | ||
|
||
/// <summary> | ||
/// Gets the default platform-specific global <see cref="MemoryAllocator"/> instance that | ||
/// serves as the default value for <see cref="Configuration.MemoryAllocator"/>. | ||
|
@@ -20,6 +24,12 @@ public abstract class MemoryAllocator | |
/// </summary> | ||
public static MemoryAllocator Default { get; } = Create(); | ||
|
||
internal long MemoryGroupAllocationLimitBytes { get; private set; } = Environment.Is64BitProcess ? | ||
4L * OneGigabyte : | ||
OneGigabyte; | ||
|
||
internal int SingleBufferAllocationLimitBytes { get; private set; } = OneGigabyte; | ||
|
||
/// <summary> | ||
/// Gets the length of the largest contiguous buffer that can be handled by this allocator instance in bytes. | ||
/// </summary> | ||
|
@@ -30,16 +40,24 @@ public abstract class MemoryAllocator | |
/// Creates a default instance of a <see cref="MemoryAllocator"/> optimized for the executing platform. | ||
/// </summary> | ||
/// <returns>The <see cref="MemoryAllocator"/>.</returns> | ||
public static MemoryAllocator Create() => | ||
new UniformUnmanagedMemoryPoolMemoryAllocator(null); | ||
public static MemoryAllocator Create() => Create(default); | ||
|
||
/// <summary> | ||
/// Creates the default <see cref="MemoryAllocator"/> using the provided options. | ||
/// </summary> | ||
/// <param name="options">The <see cref="MemoryAllocatorOptions"/>.</param> | ||
/// <returns>The <see cref="MemoryAllocator"/>.</returns> | ||
public static MemoryAllocator Create(MemoryAllocatorOptions options) => | ||
new UniformUnmanagedMemoryPoolMemoryAllocator(options.MaximumPoolSizeMegabytes); | ||
public static MemoryAllocator Create(MemoryAllocatorOptions options) | ||
{ | ||
UniformUnmanagedMemoryPoolMemoryAllocator allocator = new(options.MaximumPoolSizeMegabytes); | ||
if (options.AllocationLimitMegabytes.HasValue) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For V4 I think we should introduce There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, To secure users who still use it (mostly legacy code designed to workaround 1.* issues), I applied the default limits, but you cannot configure |
||
{ | ||
allocator.MemoryGroupAllocationLimitBytes = options.AllocationLimitMegabytes.Value * 1024 * 1024; | ||
allocator.SingleBufferAllocationLimitBytes = (int)Math.Min(allocator.SingleBufferAllocationLimitBytes, allocator.MemoryGroupAllocationLimitBytes); | ||
} | ||
|
||
return allocator; | ||
} | ||
|
||
/// <summary> | ||
/// Allocates an <see cref="IMemoryOwner{T}" />, holding a <see cref="Memory{T}"/> of length <paramref name="length"/>. | ||
|
@@ -69,10 +87,31 @@ public virtual void ReleaseRetainedResources() | |
/// <param name="options">The <see cref="AllocationOptions"/>.</param> | ||
/// <returns>A new <see cref="MemoryGroup{T}"/>.</returns> | ||
/// <exception cref="InvalidMemoryOperationException">Thrown when 'blockAlignment' converted to bytes is greater than the buffer capacity of the allocator.</exception> | ||
internal virtual MemoryGroup<T> AllocateGroup<T>( | ||
internal MemoryGroup<T> AllocateGroup<T>( | ||
long totalLength, | ||
int bufferAlignment, | ||
AllocationOptions options = AllocationOptions.None) | ||
where T : struct | ||
=> MemoryGroup<T>.Allocate(this, totalLength, bufferAlignment, options); | ||
{ | ||
long totalLengthInBytes = totalLength * Unsafe.SizeOf<T>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be possible to overflow this. I think one of our tests that allocates a 256 byte struct does so. |
||
if (totalLengthInBytes < 0) | ||
{ | ||
ThrowNotRepresentable(); | ||
} | ||
|
||
if (totalLengthInBytes > this.MemoryGroupAllocationLimitBytes) | ||
{ | ||
InvalidMemoryOperationException.ThrowAllocationOverLimitException(totalLengthInBytes, this.MemoryGroupAllocationLimitBytes); | ||
} | ||
|
||
return this.AllocateGroupCore<T>(totalLengthInBytes, totalLength, bufferAlignment, options); | ||
|
||
[DoesNotReturn] | ||
static void ThrowNotRepresentable() => | ||
throw new InvalidMemoryOperationException("Attempted to allocate a MemoryGroup of a size that is not representable."); | ||
} | ||
|
||
internal virtual MemoryGroup<T> AllocateGroupCore<T>(long totalLengthInElements, long totalLengthInBytes, int bufferAlignment, AllocationOptions options) | ||
where T : struct | ||
=> MemoryGroup<T>.Allocate(this, totalLengthInElements, bufferAlignment, options); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
// Copyright (c) Six Labors. | ||
// Copyright (c) Six Labors. | ||
// Licensed under the Six Labors Split License. | ||
|
||
namespace SixLabors.ImageSharp.Memory; | ||
|
@@ -9,6 +9,7 @@ namespace SixLabors.ImageSharp.Memory; | |
public struct MemoryAllocatorOptions | ||
{ | ||
private int? maximumPoolSizeMegabytes; | ||
private int? allocationLimitMegabytes; | ||
|
||
/// <summary> | ||
/// Gets or sets a value defining the maximum size of the <see cref="MemoryAllocator"/>'s internal memory pool | ||
|
@@ -27,4 +28,22 @@ public int? MaximumPoolSizeMegabytes | |
this.maximumPoolSizeMegabytes = value; | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Gets or sets a value defining the maximum (discontiguous) buffer size that can be allocated by the allocator in Megabytes. | ||
/// <see langword="null"/> means platform default: 1GB on 32-bit processes, 4GB on 64-bit processes. | ||
/// </summary> | ||
public int? AllocationLimitMegabytes | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You got it in the options! Nice! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this consistent with how we talk about memory across the public api? do we talk about megabytes anywhere else in the public api or is it all bytes? we want to make sure we are being consistent on our units of measure in the public api surface. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other property on |
||
get => this.allocationLimitMegabytes; | ||
set | ||
{ | ||
if (value.HasValue) | ||
{ | ||
Guard.MustBeGreaterThan(value.Value, 0, nameof(this.AllocationLimitMegabytes)); | ||
} | ||
|
||
this.allocationLimitMegabytes = value; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
// Copyright (c) Six Labors. | ||
// Copyright (c) Six Labors. | ||
// Licensed under the Six Labors Split License. | ||
|
||
using System.Buffers; | ||
using System.Runtime.CompilerServices; | ||
using SixLabors.ImageSharp.Memory.Internals; | ||
|
||
namespace SixLabors.ImageSharp.Memory; | ||
|
@@ -19,6 +20,13 @@ public override IMemoryOwner<T> Allocate<T>(int length, AllocationOptions option | |
{ | ||
Guard.MustBeGreaterThanOrEqualTo(length, 0, nameof(length)); | ||
|
||
int lengthInBytes = length * Unsafe.SizeOf<T>(); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For V4 I'd like to have this in |
||
if (lengthInBytes > this.SingleBufferAllocationLimitBytes) | ||
{ | ||
InvalidMemoryOperationException.ThrowAllocationOverLimitException(lengthInBytes, this.SingleBufferAllocationLimitBytes); | ||
} | ||
|
||
return new BasicArrayBuffer<T>(new T[length]); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
// Copyright (c) Six Labors. | ||
// Licensed under the Six Labors Split License. | ||
|
||
using System.Diagnostics.CodeAnalysis; | ||
|
||
namespace SixLabors.ImageSharp.Memory; | ||
|
||
/// <summary> | ||
|
@@ -24,4 +26,8 @@ public InvalidMemoryOperationException(string message) | |
public InvalidMemoryOperationException() | ||
{ | ||
} | ||
|
||
[DoesNotReturn] | ||
internal static void ThrowAllocationOverLimitException(long length, long limit) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this sitting here. |
||
throw new InvalidMemoryOperationException($"Attempted to allocate a buffer of length={length} that exceeded the limit {limit}."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there is no good reason to set this to 1GB. Contiguous buffers have a natural limit of
int.MaxValue ~ 2GB
so I think we should set the contiguous limit tomin(int.MaxValue, MemoryAllocatorOptions.AllocationLimitMegabytes * 1MB)
. That would get rid of all obscure logic and make the semantics ofMemoryAllocatorOptions.AllocationLimitMegabytes
very obvious.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I like that. The only risk we have with this stuff is in our decoders which can be easily mitigated with some care (PNG tweaks)