Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
<InternalsVisibleTo Include="ImageSharp.Benchmarks" Key="$(SixLaborsPublicKey)" />
<InternalsVisibleTo Include="ImageSharp.Tests.ProfilingSandbox" Key="$(SixLaborsPublicKey)" />
<InternalsVisibleTo Include="SixLabors.ImageSharp.Tests" Key="$(SixLaborsPublicKey)" />
<InternalsVisibleTo Include="SixLabors.ImageSharp.Drawing.Tests" Key="$(SixLaborsPublicKey)" />
</ItemGroup>

</Project>
114 changes: 86 additions & 28 deletions src/ImageSharp/Diagnostics/MemoryDiagnostics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,8 @@ namespace SixLabors.ImageSharp.Diagnostics
/// </summary>
public static class MemoryDiagnostics
{
private static int totalUndisposedAllocationCount;

private static UndisposedAllocationDelegate undisposedAllocation;
private static int undisposedAllocationSubscriptionCounter;
internal static readonly InteralMemoryDiagnostics Default = new();
private static AsyncLocal<InteralMemoryDiagnostics> localInstance = null;
private static readonly object SyncRoot = new();

/// <summary>
Expand All @@ -28,56 +26,116 @@ public static class MemoryDiagnostics
/// </summary>
public static event UndisposedAllocationDelegate UndisposedAllocation
{
add
add => Current.UndisposedAllocation += value;
remove => Current.UndisposedAllocation -= value;
}

internal static InteralMemoryDiagnostics Current
{
get
{
lock (SyncRoot)
if (localInstance != null && localInstance.Value != null)
{
undisposedAllocationSubscriptionCounter++;
undisposedAllocation += value;
return localInstance.Value;
}

return Default;
}

remove
set
{
lock (SyncRoot)
if (localInstance == null)
{
undisposedAllocation -= value;
undisposedAllocationSubscriptionCounter--;
lock (SyncRoot)
{
localInstance ??= new AsyncLocal<InteralMemoryDiagnostics>();
}
}

localInstance.Value = value;
}
}

/// <summary>
/// Gets a value indicating the total number of memory resource objects leaked to the finalizer.
/// </summary>
public static int TotalUndisposedAllocationCount => totalUndisposedAllocationCount;
public static int TotalUndisposedAllocationCount => Current.TotalUndisposedAllocationCount;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TotalUndisposedAllocationCount was intended to report global allocation count. If I get the docs of AsyncLocal<T> right, the PR changes the semantics, so it becomes specific to an asynchronous execution flow, and will reset outside of the flow. I don't think this public behavior change is beneficial for the users of MemoryDiagnostics.TotalUndisposedAllocationCount . We need a separate API for MemoryAllocatorValidator.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It way its setup it will only report the local context when its been overwritten in tests (internal api) by default the backing fields are backs by the MemoryDiagnostics.Default static instance which will be used in production usage outside of testing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now, I missed the purpose of the branching. Though probably still better to keep the internal API as minimalistic and non-intrusive as possible.


internal static bool UndisposedAllocationSubscribed => Volatile.Read(ref undisposedAllocationSubscriptionCounter) > 0;
internal static bool UndisposedAllocationSubscribed => Current.UndisposedAllocationSubscribed;

internal static void IncrementTotalUndisposedAllocationCount() =>
Interlocked.Increment(ref totalUndisposedAllocationCount);
internal static void IncrementTotalUndisposedAllocationCount() => Current.IncrementTotalUndisposedAllocationCount();

internal static void DecrementTotalUndisposedAllocationCount() =>
Interlocked.Decrement(ref totalUndisposedAllocationCount);
internal static void DecrementTotalUndisposedAllocationCount() => Current.DecrementTotalUndisposedAllocationCount();

internal static void RaiseUndisposedMemoryResource(string allocationStackTrace)
=> Current.RaiseUndisposedMemoryResource(allocationStackTrace);

internal class InteralMemoryDiagnostics
{
if (undisposedAllocation is null)
private int totalUndisposedAllocationCount;

private UndisposedAllocationDelegate undisposedAllocation;
private int undisposedAllocationSubscriptionCounter;
private readonly object syncRoot = new();

/// <summary>
/// Fires when an ImageSharp object's undisposed memory resource leaks to the finalizer.
/// The event brings significant overhead, and is intended to be used for troubleshooting only.
/// For production diagnostics, use <see cref="TotalUndisposedAllocationCount"/>.
/// </summary>
public event UndisposedAllocationDelegate UndisposedAllocation
{
return;
add
{
lock (this.syncRoot)
{
this.undisposedAllocationSubscriptionCounter++;
this.undisposedAllocation += value;
}
}

remove
{
lock (this.syncRoot)
{
this.undisposedAllocation -= value;
this.undisposedAllocationSubscriptionCounter--;
}
}
}

// Schedule on the ThreadPool, to avoid user callback messing up the finalizer thread.
/// <summary>
/// Gets a value indicating the total number of memory resource objects leaked to the finalizer.
/// </summary>
public int TotalUndisposedAllocationCount => this.totalUndisposedAllocationCount;

internal bool UndisposedAllocationSubscribed => Volatile.Read(ref this.undisposedAllocationSubscriptionCounter) > 0;

internal void IncrementTotalUndisposedAllocationCount() =>
Interlocked.Increment(ref this.totalUndisposedAllocationCount);

internal void DecrementTotalUndisposedAllocationCount() =>
Interlocked.Decrement(ref this.totalUndisposedAllocationCount);

internal void RaiseUndisposedMemoryResource(string allocationStackTrace)
{
if (this.undisposedAllocation is null)
{
return;
}

// Schedule on the ThreadPool, to avoid user callback messing up the finalizer thread.
#if NETSTANDARD2_1 || NETCOREAPP2_1_OR_GREATER
ThreadPool.QueueUserWorkItem(
stackTrace => undisposedAllocation?.Invoke(stackTrace),
allocationStackTrace,
preferLocal: false);
ThreadPool.QueueUserWorkItem(
stackTrace => this.undisposedAllocation?.Invoke(stackTrace),
allocationStackTrace,
preferLocal: false);
#else
ThreadPool.QueueUserWorkItem(
stackTrace => undisposedAllocation?.Invoke((string)stackTrace),
allocationStackTrace);
ThreadPool.QueueUserWorkItem(
stackTrace => this.undisposedAllocation?.Invoke((string)stackTrace),
allocationStackTrace);
#endif
}
}
}
}
9 changes: 8 additions & 1 deletion src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,12 @@ public BmpDecoderCore(Configuration configuration, IBmpDecoderOptions options)
public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken cancellationToken)
where TPixel : unmanaged, IPixel<TPixel>
{
Image<TPixel> image = null;
try
{
int bytesPerColorMapEntry = this.ReadImageHeaders(stream, out bool inverted, out byte[] palette);

var image = new Image<TPixel>(this.Configuration, this.infoHeader.Width, this.infoHeader.Height, this.metadata);
image = new Image<TPixel>(this.Configuration, this.infoHeader.Width, this.infoHeader.Height, this.metadata);

Buffer2D<TPixel> pixels = image.GetRootFramePixelBuffer();

Expand Down Expand Up @@ -193,8 +194,14 @@ public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken
}
catch (IndexOutOfRangeException e)
{
image?.Dispose();
throw new ImageFormatException("Bitmap does not have a valid format.", e);
}
catch
{
image?.Dispose();
throw;
}
}

/// <inheritdoc />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ public Buffer2D<TPixel> GetPixelBuffer(CancellationToken cancellationToken)
}
}

return this.pixelBuffer;
var buffer = this.pixelBuffer;
this.pixelBuffer = null;
return buffer;
}

/// <inheritdoc/>
Expand Down Expand Up @@ -210,6 +212,7 @@ public void Dispose()

this.rgbBuffer?.Dispose();
this.paddedProxyPixelRow?.Dispose();
this.pixelBuffer?.Dispose();
}
}
}
35 changes: 21 additions & 14 deletions src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -187,20 +187,27 @@ public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken
where TPixel : unmanaged, IPixel<TPixel>
{
using var spectralConverter = new SpectralConverter<TPixel>(this.Configuration);

var scanDecoder = new HuffmanScanDecoder(stream, spectralConverter, cancellationToken);

this.ParseStream(stream, scanDecoder, cancellationToken);
this.InitExifProfile();
this.InitIccProfile();
this.InitIptcProfile();
this.InitXmpProfile();
this.InitDerivedMetadataProperties();

return new Image<TPixel>(
this.Configuration,
spectralConverter.GetPixelBuffer(cancellationToken),
this.Metadata);
try
{
var scanDecoder = new HuffmanScanDecoder(stream, spectralConverter, cancellationToken);

this.ParseStream(stream, scanDecoder, cancellationToken);
this.InitExifProfile();
this.InitIccProfile();
this.InitIptcProfile();
this.InitXmpProfile();
this.InitDerivedMetadataProperties();

return new Image<TPixel>(
this.Configuration,
spectralConverter.GetPixelBuffer(cancellationToken),
this.Metadata);
}
catch
{
this.Frame?.Dispose();
throw;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to do a try-catch here I believe as it is handled by the caller here:

public Image<TPixel> Decode<TPixel>(Configuration configuration, Stream stream, CancellationToken cancellationToken)
where TPixel : unmanaged, IPixel<TPixel>
{
Guard.NotNull(stream, nameof(stream));
using var decoder = new JpegDecoderCore(configuration, this);
return decoder.Decode<TPixel>(configuration, stream, cancellationToken);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah looks like you are right... I think I must have added it on first pass trying to find the leaky code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very tricky because jpeg is the only decoder which has a disposable internal DecoderCore implementation :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make it so it’s doesn’t implement IDisposable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Considering it's only used for Frame field which is a class field for the sake of not passing it to a bunch of methods we can make it non-disposable for consistency.

If you do so, try-catch must be replaced by try-finally or Frame would leak in a no-exception scenario. Also don't forget about Identify method - it also creates a Frame field so it should have a proper try-finally too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Ace, that’s what I thought. That can be a separate PR though.

}

/// <inheritdoc/>
Expand Down
12 changes: 12 additions & 0 deletions src/ImageSharp/Formats/Png/PngDecoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -227,10 +227,16 @@ public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken

return image;
}
catch
{
image?.Dispose();
throw;
}
finally
{
this.scanline?.Dispose();
this.previousScanline?.Dispose();
this.nextChunk?.Data?.Dispose();
}
}

Expand Down Expand Up @@ -472,6 +478,8 @@ private void InitializeImage<TPixel>(ImageMetadata metadata, out Image<TPixel> i
this.bytesPerSample = this.header.BitDepth / 8;
}

this.previousScanline?.Dispose();
this.scanline?.Dispose();
this.previousScanline = this.memoryAllocator.Allocate<byte>(this.bytesPerScanline, AllocationOptions.Clean);
this.scanline = this.Configuration.MemoryAllocator.Allocate<byte>(this.bytesPerScanline, AllocationOptions.Clean);
}
Expand Down Expand Up @@ -1359,6 +1367,7 @@ private int ReadNextDataChunk()
{
if (chunk.Type == PngChunkType.Data)
{
chunk.Data?.Dispose();
return chunk.Length;
}

Expand Down Expand Up @@ -1453,6 +1462,9 @@ private void ValidateChunk(in PngChunk chunk)
if (validCrc != inputCrc)
{
string chunkTypeName = Encoding.ASCII.GetString(chunkType);

// ensure when throwing we dispose the data back to the memory allocator
chunk.Data?.Dispose();
PngThrowHelper.ThrowInvalidChunkCrc(chunkTypeName);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ protected override void Decompress(BufferedReadStream stream, int byteCount, int
jpegDecoder.ParseStream(stream, scanDecoderGray, CancellationToken.None);

// TODO: Should we pass through the CancellationToken from the tiff decoder?
CopyImageBytesToBuffer(buffer, spectralConverterGray.GetPixelBuffer(CancellationToken.None));
using var decompressedBuffer = spectralConverterGray.GetPixelBuffer(CancellationToken.None);
CopyImageBytesToBuffer(buffer, decompressedBuffer);
Comment on lines +68 to +69
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also have separate PR-s for the Jpeg+Tiff+WebP+Bmp fixes or just for PNG?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the decoder facing fixes are actually in here...but we will need a set of follow up PRs to get full coverage.

break;
}

Expand All @@ -81,7 +82,8 @@ protected override void Decompress(BufferedReadStream stream, int byteCount, int
jpegDecoder.ParseStream(stream, scanDecoder, CancellationToken.None);

// TODO: Should we pass through the CancellationToken from the tiff decoder?
CopyImageBytesToBuffer(buffer, spectralConverter.GetPixelBuffer(CancellationToken.None));
using var decompressedBuffer = spectralConverter.GetPixelBuffer(CancellationToken.None);
CopyImageBytesToBuffer(buffer, decompressedBuffer);
break;
}

Expand Down
Loading