From 77bb28711613023cea454820b8f0598aae8644ed Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sat, 9 Apr 2022 20:20:36 +0100 Subject: [PATCH] xunit helper to track undisposed memory --- .../Diagnostics/MemoryDiagnostics.cs | 110 +++++++++++++----- .../Formats/Png/PngDecoderTests.cs | 4 + .../Image/ImageTests.LoadPixelData.cs | 1 + .../Image/LargeImageIntegrationTests.cs | 2 + .../MemoryAllocatorValidator.cs | 43 +++++++ .../ImageProviders/FileProvider.cs | 8 +- ...idateDisposedMemoryAllocationsAttribute.cs | 36 ++++++ 7 files changed, 177 insertions(+), 27 deletions(-) create mode 100644 tests/ImageSharp.Tests/MemoryAllocatorValidator.cs create mode 100644 tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index d83e737f12..6af2544138 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -15,10 +15,8 @@ namespace SixLabors.ImageSharp.Diagnostics /// 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 localInstance = null; private static readonly object SyncRoot = new(); /// @@ -28,56 +26,116 @@ namespace SixLabors.ImageSharp.Diagnostics /// 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(); + } } + + localInstance.Value = value; } } /// /// Gets a value indicating the total number of memory resource objects leaked to the finalizer. /// - public static int TotalUndisposedAllocationCount => totalUndisposedAllocationCount; + public static int TotalUndisposedAllocationCount => Current.TotalUndisposedAllocationCount; - 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(); + + /// + /// 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 . + /// + 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. + /// + /// Gets a value indicating the total number of memory resource objects leaked to the finalizer. + /// + 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), + stackTrace => this.undisposedAllocation?.Invoke((string)stackTrace), allocationStackTrace); #endif + } } } } diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index 752036126f..0a796b5143 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -103,10 +103,14 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png [Theory] [WithFileCollection(nameof(CommonTestImages), PixelTypes.Rgba32)] + [ValidateDisposedMemoryAllocations] public void Decode(TestImageProvider provider) where TPixel : unmanaged, IPixel { using Image image = provider.GetImage(PngDecoder); + //var testFile = TestFile.Create(provider.SourceFileOrDescription); + //using Image image = Image.Load(provider.Configuration, testFile.Bytes, PngDecoder); + image.DebugSave(provider); image.CompareToOriginal(provider, ImageComparer.Exact); } diff --git a/tests/ImageSharp.Tests/Image/ImageTests.LoadPixelData.cs b/tests/ImageSharp.Tests/Image/ImageTests.LoadPixelData.cs index a4044b906c..7683ee6889 100644 --- a/tests/ImageSharp.Tests/Image/ImageTests.LoadPixelData.cs +++ b/tests/ImageSharp.Tests/Image/ImageTests.LoadPixelData.cs @@ -14,6 +14,7 @@ namespace SixLabors.ImageSharp.Tests [Theory] [InlineData(false)] [InlineData(true)] + [ValidateDisposedMemoryAllocations] public void FromPixels(bool useSpan) { Rgba32[] data = { Color.Black, Color.White, Color.White, Color.Black, }; diff --git a/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs b/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs index 357d02be4b..ce1f902e59 100644 --- a/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs +++ b/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs @@ -55,6 +55,8 @@ namespace SixLabors.ImageSharp.Tests static void RunTest(string formatInner) { + using IDisposable mem = MemoryAllocatorValidator.MonitorAllocations(); + Configuration configuration = Configuration.Default.Clone(); configuration.PreferContiguousImageBuffers = true; IImageEncoder encoder = configuration.ImageFormatsManager.FindEncoder( diff --git a/tests/ImageSharp.Tests/MemoryAllocatorValidator.cs b/tests/ImageSharp.Tests/MemoryAllocatorValidator.cs new file mode 100644 index 0000000000..2c3d98eb13 --- /dev/null +++ b/tests/ImageSharp.Tests/MemoryAllocatorValidator.cs @@ -0,0 +1,43 @@ +// Copyright (c) Six Labors. +// Licensed under the Apache License, Version 2.0. + +using System; +using System.Diagnostics; +using SixLabors.ImageSharp.Diagnostics; +using Xunit; + +namespace SixLabors.ImageSharp.Tests +{ + public static class MemoryAllocatorValidator + { + public static IDisposable MonitorAllocations(int max = 0) + { + MemoryDiagnostics.Current = new(); + return new TestMemoryAllocatorDisposable(max); + } + + public static void ValidateAllocation(int max = 0) + { + var count = MemoryDiagnostics.TotalUndisposedAllocationCount; + var pass = count <= max; + Assert.True(pass, $"Expected a max of {max} undisposed buffers but found {count}"); + + if (count > 0) + { + Debug.WriteLine("We should have Zero undisposed memory allocations."); + } + + MemoryDiagnostics.Current = null; + } + + public struct TestMemoryAllocatorDisposable : IDisposable + { + private readonly int max; + + public TestMemoryAllocatorDisposable(int max) => this.max = max; + + public void Dispose() + => ValidateAllocation(this.max); + } + } +} diff --git a/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs b/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs index e2e7d73bc6..3675356c38 100644 --- a/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs +++ b/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs @@ -7,6 +7,7 @@ using System.Collections.Generic; using System.IO; using System.Reflection; using System.Threading.Tasks; +using SixLabors.ImageSharp.Diagnostics; using SixLabors.ImageSharp.Formats; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; @@ -158,8 +159,13 @@ namespace SixLabors.ImageSharp.Tests return this.LoadImage(decoder); } - var key = new Key(this.PixelType, this.FilePath, decoder); + // do cache so we can track allocation correctly when validating memory + if (MemoryDiagnostics.Current != MemoryDiagnostics.Default) + { + return this.LoadImage(decoder); + } + var key = new Key(this.PixelType, this.FilePath, decoder); Image cachedImage = Cache.GetOrAdd(key, _ => this.LoadImage(decoder)); return cachedImage.Clone(this.Configuration); diff --git a/tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs b/tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs new file mode 100644 index 0000000000..95d13b5954 --- /dev/null +++ b/tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs @@ -0,0 +1,36 @@ +// Copyright (c) Six Labors. +// Licensed under the Apache License, Version 2.0. + +using System; +using System.Diagnostics; +using System.Reflection; +using Xunit.Sdk; + +namespace SixLabors.ImageSharp.Tests +{ + [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] + public class ValidateDisposedMemoryAllocationsAttribute : BeforeAfterTestAttribute + { + private readonly int max = 0; + + public ValidateDisposedMemoryAllocationsAttribute() + : this(0) + { + } + + public ValidateDisposedMemoryAllocationsAttribute(int max) + { + this.max = max; + if (max > 0) + { + Debug.WriteLine("Needs fixing, we shoudl have Zero undisposed memory allocations."); + } + } + + public override void Before(MethodInfo methodUnderTest) + => MemoryAllocatorValidator.MonitorAllocations(this.max); // the disposable isn't important cause the validate below does the same thing + + public override void After(MethodInfo methodUnderTest) + => MemoryAllocatorValidator.ValidateAllocation(this.max); + } +}