From 4736288e0456353408ba14539a10c434ce269fe3 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Wed, 26 Jan 2022 00:14:47 +0100 Subject: [PATCH 1/8] API skeleton + tests for TotalUndisposedAllocationCount --- .../Diagnostics/MemoryDiagnostics.cs | 22 +++++ .../Allocators/MemoryDiagnosticsTests.cs | 94 +++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 src/ImageSharp/Diagnostics/MemoryDiagnostics.cs create mode 100644 tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs new file mode 100644 index 000000000..fa9a1b8c2 --- /dev/null +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -0,0 +1,22 @@ +// Copyright (c) Six Labors. +// Licensed under the Apache License, Version 2.0. + +using System; + +namespace SixLabors.ImageSharp.Diagnostics +{ + public readonly struct MemoryInfo + { + internal MemoryInfo(long totalUndisposedAllocationCount) + => this.TotalUndisposedAllocationCount = totalUndisposedAllocationCount; + + public long TotalUndisposedAllocationCount { get; } + } + + public static class MemoryDiagnostics + { + public static MemoryInfo GetMemoryInfo() => throw new NotImplementedException(); + + public static bool EnableStrictDisposeWatcher { get; set; } + } +} diff --git a/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs new file mode 100644 index 000000000..11a70d1cb --- /dev/null +++ b/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs @@ -0,0 +1,94 @@ +// Copyright (c) Six Labors. +// Licensed under the Apache License, Version 2.0. + +using System; +using System.Buffers; +using System.Collections.Generic; +using System.Runtime.CompilerServices; +using Microsoft.DotNet.RemoteExecutor; +using SixLabors.ImageSharp.Diagnostics; +using SixLabors.ImageSharp.Memory; +using Xunit; + +namespace SixLabors.ImageSharp.Tests.Memory.Allocators +{ + public class MemoryDiagnosticsTests + { + private const int OneMb = 1 << 20; + + private static MemoryAllocator Allocator => Configuration.Default.MemoryAllocator; + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void AllocateDispose_Maintains_TotalUndisposedLogicalAllocationCount(bool isGroupOuter) + { + RemoteExecutor.Invoke(RunTest, isGroupOuter.ToString()).Dispose(); + + static void RunTest(string isGroupInner) + { + bool isGroup = bool.Parse(isGroupInner); + List buffers = new(); + + Assert.Equal(0, MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount); + for (int length = 1024; length <= 64 * OneMb; length *= 2) + { + long cntBefore = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + IDisposable buffer = isGroup ? + Allocator.AllocateGroup(length, 1024) : + Allocator.Allocate(length); + buffers.Add(buffer); + long cntAfter = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + Assert.True(cntAfter > cntBefore); + } + + foreach (IDisposable buffer in buffers) + { + long cntBefore = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + buffer.Dispose(); + long cntAfter = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + Assert.True(cntAfter < cntBefore); + } + + Assert.Equal(0, MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount); + } + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void BufferAndGroupFinalizer_DoesNotReduce_TotalUndisposedLogicalAllocationCount(bool isGroupOuter) + { + RemoteExecutor.Invoke(RunTest, isGroupOuter.ToString()).Dispose(); + + static void RunTest(string isGroupInner) + { + bool isGroup = bool.Parse(isGroupInner); + Assert.Equal(0, MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount); + for (int length = 1024; length <= 64 * OneMb; length *= 2) + { + long cntBefore = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + AllocateAndForget(length, isGroup); + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + long cntAfter = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + Assert.True(cntAfter > cntBefore); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void AllocateAndForget(int length, bool isGroup) + { + if (isGroup) + { + _ = Allocator.AllocateGroup(length, 1024); + } + else + { + _ = Allocator.Allocate(length); + } + } + } + } +} From b16fd4c6859e0ebce36222c958c0d93631784702 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Wed, 26 Jan 2022 01:44:55 +0100 Subject: [PATCH 2/8] implemented TotalUndisposedAllocationCount --- .../Diagnostics/MemoryDiagnostics.cs | 19 ++++---- src/ImageSharp/Diagnostics/MemoryInfo.cs | 13 ++++++ ...rd.cs => RefCountedMemoryLifetimeGuard.cs} | 46 +++++++++++-------- .../Internals/SharedArrayPoolBuffer{T}.cs | 2 +- ...iformUnmanagedMemoryPool.LifetimeGuards.cs | 4 +- .../Internals/UnmanagedBufferLifetimeGuard.cs | 2 +- .../MemoryGroup{T}.Owned.cs | 2 +- .../Allocators/MemoryDiagnosticsTests.cs | 1 - .../RefCountedLifetimeGuardTests.cs | 2 +- 9 files changed, 57 insertions(+), 34 deletions(-) create mode 100644 src/ImageSharp/Diagnostics/MemoryInfo.cs rename src/ImageSharp/Memory/Allocators/Internals/{RefCountedLifetimeGuard.cs => RefCountedMemoryLifetimeGuard.cs} (61%) diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index fa9a1b8c2..419f7531f 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -2,21 +2,22 @@ // Licensed under the Apache License, Version 2.0. using System; +using System.Threading; namespace SixLabors.ImageSharp.Diagnostics { - public readonly struct MemoryInfo - { - internal MemoryInfo(long totalUndisposedAllocationCount) - => this.TotalUndisposedAllocationCount = totalUndisposedAllocationCount; - - public long TotalUndisposedAllocationCount { get; } - } - public static class MemoryDiagnostics { - public static MemoryInfo GetMemoryInfo() => throw new NotImplementedException(); + private static int totalUndisposedAllocationCount; + + public static MemoryInfo GetMemoryInfo() => new MemoryInfo(totalUndisposedAllocationCount); public static bool EnableStrictDisposeWatcher { get; set; } + + internal static void IncrementTotalUndisposedAllocationCount() => + Interlocked.Increment(ref totalUndisposedAllocationCount); + + internal static void DecrementTotalUndisposedAllocationCount() => + Interlocked.Decrement(ref totalUndisposedAllocationCount); } } diff --git a/src/ImageSharp/Diagnostics/MemoryInfo.cs b/src/ImageSharp/Diagnostics/MemoryInfo.cs new file mode 100644 index 000000000..54ae83bd0 --- /dev/null +++ b/src/ImageSharp/Diagnostics/MemoryInfo.cs @@ -0,0 +1,13 @@ +// Copyright (c) Six Labors. +// Licensed under the Apache License, Version 2.0. + +namespace SixLabors.ImageSharp.Diagnostics +{ + public readonly struct MemoryInfo + { + internal MemoryInfo(long totalUndisposedAllocationCount) + => this.TotalUndisposedAllocationCount = totalUndisposedAllocationCount; + + public long TotalUndisposedAllocationCount { get; } + } +} diff --git a/src/ImageSharp/Memory/Allocators/Internals/RefCountedLifetimeGuard.cs b/src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs similarity index 61% rename from src/ImageSharp/Memory/Allocators/Internals/RefCountedLifetimeGuard.cs rename to src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs index 61682aa56..534cec0f8 100644 --- a/src/ImageSharp/Memory/Allocators/Internals/RefCountedLifetimeGuard.cs +++ b/src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs @@ -4,42 +4,33 @@ using System; using System.Runtime.InteropServices; using System.Threading; +using SixLabors.ImageSharp.Diagnostics; namespace SixLabors.ImageSharp.Memory.Internals { /// - /// Implements reference counting lifetime guard mechanism similar to the one provided by , - /// but without the restriction of the guarded object being a handle. + /// Implements reference counting lifetime guard mechanism for memory resources + /// also maintaining the current value of . /// - internal abstract class RefCountedLifetimeGuard : IDisposable + internal abstract class RefCountedMemoryLifetimeGuard : IDisposable { private int refCount = 1; private int disposed; private int released; - ~RefCountedLifetimeGuard() + public RefCountedMemoryLifetimeGuard() => MemoryDiagnostics.IncrementTotalUndisposedAllocationCount(); + + ~RefCountedMemoryLifetimeGuard() { Interlocked.Exchange(ref this.disposed, 1); - this.ReleaseRef(); + this.ReleaseRef(true); } public bool IsDisposed => this.disposed == 1; public void AddRef() => Interlocked.Increment(ref this.refCount); - public void ReleaseRef() - { - Interlocked.Decrement(ref this.refCount); - if (this.refCount == 0) - { - int wasReleased = Interlocked.Exchange(ref this.released, 1); - - if (wasReleased == 0) - { - this.Release(); - } - } - } + public void ReleaseRef() => this.ReleaseRef(false); public void Dispose() { @@ -52,5 +43,24 @@ namespace SixLabors.ImageSharp.Memory.Internals } protected abstract void Release(); + + private void ReleaseRef(bool finalizing) + { + Interlocked.Decrement(ref this.refCount); + if (this.refCount == 0) + { + int wasReleased = Interlocked.Exchange(ref this.released, 1); + + if (wasReleased == 0) + { + if (!finalizing) + { + MemoryDiagnostics.DecrementTotalUndisposedAllocationCount(); + } + + this.Release(); + } + } + } } } diff --git a/src/ImageSharp/Memory/Allocators/Internals/SharedArrayPoolBuffer{T}.cs b/src/ImageSharp/Memory/Allocators/Internals/SharedArrayPoolBuffer{T}.cs index 9302c67e7..2ea76da95 100644 --- a/src/ImageSharp/Memory/Allocators/Internals/SharedArrayPoolBuffer{T}.cs +++ b/src/ImageSharp/Memory/Allocators/Internals/SharedArrayPoolBuffer{T}.cs @@ -60,7 +60,7 @@ namespace SixLabors.ImageSharp.Memory.Internals } } - private sealed class LifetimeGuard : RefCountedLifetimeGuard + private sealed class LifetimeGuard : RefCountedMemoryLifetimeGuard { private byte[] array; diff --git a/src/ImageSharp/Memory/Allocators/Internals/UniformUnmanagedMemoryPool.LifetimeGuards.cs b/src/ImageSharp/Memory/Allocators/Internals/UniformUnmanagedMemoryPool.LifetimeGuards.cs index 666b24855..151fef69c 100644 --- a/src/ImageSharp/Memory/Allocators/Internals/UniformUnmanagedMemoryPool.LifetimeGuards.cs +++ b/src/ImageSharp/Memory/Allocators/Internals/UniformUnmanagedMemoryPool.LifetimeGuards.cs @@ -20,9 +20,9 @@ namespace SixLabors.ImageSharp.Memory.Internals return buffer; } - public RefCountedLifetimeGuard CreateGroupLifetimeGuard(UnmanagedMemoryHandle[] handles) => new GroupLifetimeGuard(this, handles); + public RefCountedMemoryLifetimeGuard CreateGroupLifetimeGuard(UnmanagedMemoryHandle[] handles) => new GroupLifetimeGuard(this, handles); - private sealed class GroupLifetimeGuard : RefCountedLifetimeGuard + private sealed class GroupLifetimeGuard : RefCountedMemoryLifetimeGuard { private readonly UniformUnmanagedMemoryPool pool; private readonly UnmanagedMemoryHandle[] handles; diff --git a/src/ImageSharp/Memory/Allocators/Internals/UnmanagedBufferLifetimeGuard.cs b/src/ImageSharp/Memory/Allocators/Internals/UnmanagedBufferLifetimeGuard.cs index 5f0759f20..822112024 100644 --- a/src/ImageSharp/Memory/Allocators/Internals/UnmanagedBufferLifetimeGuard.cs +++ b/src/ImageSharp/Memory/Allocators/Internals/UnmanagedBufferLifetimeGuard.cs @@ -6,7 +6,7 @@ namespace SixLabors.ImageSharp.Memory.Internals /// /// Defines a strategy for managing unmanaged memory ownership. /// - internal abstract class UnmanagedBufferLifetimeGuard : RefCountedLifetimeGuard + internal abstract class UnmanagedBufferLifetimeGuard : RefCountedMemoryLifetimeGuard { private UnmanagedMemoryHandle handle; diff --git a/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.Owned.cs b/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.Owned.cs index 21faf8e56..01aac3148 100644 --- a/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.Owned.cs +++ b/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.Owned.cs @@ -18,7 +18,7 @@ namespace SixLabors.ImageSharp.Memory public sealed class Owned : MemoryGroup, IEnumerable> { private IMemoryOwner[] memoryOwners; - private RefCountedLifetimeGuard groupLifetimeGuard; + private RefCountedMemoryLifetimeGuard groupLifetimeGuard; public Owned(IMemoryOwner[] memoryOwners, int bufferLength, long totalLength, bool swappable) : base(bufferLength, totalLength) diff --git a/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs index 11a70d1cb..55f14fdfa 100644 --- a/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. using System; -using System.Buffers; using System.Collections.Generic; using System.Runtime.CompilerServices; using Microsoft.DotNet.RemoteExecutor; diff --git a/tests/ImageSharp.Tests/Memory/Allocators/RefCountedLifetimeGuardTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/RefCountedLifetimeGuardTests.cs index 7fb3b7b7b..4b808e863 100644 --- a/tests/ImageSharp.Tests/Memory/Allocators/RefCountedLifetimeGuardTests.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/RefCountedLifetimeGuardTests.cs @@ -110,7 +110,7 @@ namespace SixLabors.ImageSharp.Tests.Memory.Allocators } } - private class MockLifetimeGuard : RefCountedLifetimeGuard + private class MockLifetimeGuard : RefCountedMemoryLifetimeGuard { public int ReleaseInvocationCount { get; private set; } From 9d155a55aaddd488896537635cb29338cb9e2431 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Tue, 1 Feb 2022 01:41:23 +0100 Subject: [PATCH 3/8] finalize and document the API --- .../Diagnostics/MemoryDiagnostics.cs | 44 +++++++++++++- src/ImageSharp/Diagnostics/MemoryInfo.cs | 13 ---- .../RefCountedMemoryLifetimeGuard.cs | 17 +++++- .../Allocators/MemoryDiagnosticsTests.cs | 59 ++++++++++++++----- 4 files changed, 101 insertions(+), 32 deletions(-) delete mode 100644 src/ImageSharp/Diagnostics/MemoryInfo.cs diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index 419f7531f..6bd81d7e5 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -6,18 +6,58 @@ using System.Threading; namespace SixLabors.ImageSharp.Diagnostics { + /// + /// Represents the method to handle . + /// + public delegate void UndisposedMemoryResourceDelegate(string allocationStackTrace); + + /// + /// Utilities to track memory usage and detect memory leaks from not disposing ImageSharp objects. + /// public static class MemoryDiagnostics { private static int totalUndisposedAllocationCount; - public static MemoryInfo GetMemoryInfo() => new MemoryInfo(totalUndisposedAllocationCount); + private static UndisposedMemoryResourceDelegate undisposedMemoryResource; + private static int undisposedMemoryResourceSubscriptionCounter; + + /// + /// 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 static event UndisposedMemoryResourceDelegate UndisposedAllocation + { + add + { + Interlocked.Increment(ref undisposedMemoryResourceSubscriptionCounter); + undisposedMemoryResource += value; + } + + remove + { + undisposedMemoryResource -= value; + Interlocked.Decrement(ref undisposedMemoryResourceSubscriptionCounter); + } + } - public static bool EnableStrictDisposeWatcher { get; set; } + /// + /// Gets a value indicating the total number of memory resource objects leaked to the finalizer. + /// + public static int TotalUndisposedAllocationCount => totalUndisposedAllocationCount; + + internal static bool MemoryResourceLeakedSubscribed => undisposedMemoryResourceSubscriptionCounter > 0; internal static void IncrementTotalUndisposedAllocationCount() => Interlocked.Increment(ref totalUndisposedAllocationCount); internal static void DecrementTotalUndisposedAllocationCount() => Interlocked.Decrement(ref totalUndisposedAllocationCount); + + internal static void RaiseUndisposedMemoryResource(string allocationStackTrace) + { + // Schedule on the ThreadPool, to avoid user callback messing up the finalizer thread. + ThreadPool.QueueUserWorkItem(_ => undisposedMemoryResource?.Invoke(allocationStackTrace)); + } } } diff --git a/src/ImageSharp/Diagnostics/MemoryInfo.cs b/src/ImageSharp/Diagnostics/MemoryInfo.cs deleted file mode 100644 index 54ae83bd0..000000000 --- a/src/ImageSharp/Diagnostics/MemoryInfo.cs +++ /dev/null @@ -1,13 +0,0 @@ -// Copyright (c) Six Labors. -// Licensed under the Apache License, Version 2.0. - -namespace SixLabors.ImageSharp.Diagnostics -{ - public readonly struct MemoryInfo - { - internal MemoryInfo(long totalUndisposedAllocationCount) - => this.TotalUndisposedAllocationCount = totalUndisposedAllocationCount; - - public long TotalUndisposedAllocationCount { get; } - } -} diff --git a/src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs b/src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs index 534cec0f8..333ef85c2 100644 --- a/src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs +++ b/src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs @@ -10,15 +10,24 @@ namespace SixLabors.ImageSharp.Memory.Internals { /// /// Implements reference counting lifetime guard mechanism for memory resources - /// also maintaining the current value of . + /// and maintains the value of . /// internal abstract class RefCountedMemoryLifetimeGuard : IDisposable { private int refCount = 1; private int disposed; private int released; + private string allocationStackTrace; - public RefCountedMemoryLifetimeGuard() => MemoryDiagnostics.IncrementTotalUndisposedAllocationCount(); + protected RefCountedMemoryLifetimeGuard() + { + if (MemoryDiagnostics.MemoryResourceLeakedSubscribed) + { + this.allocationStackTrace = Environment.StackTrace; + } + + MemoryDiagnostics.IncrementTotalUndisposedAllocationCount(); + } ~RefCountedMemoryLifetimeGuard() { @@ -57,6 +66,10 @@ namespace SixLabors.ImageSharp.Memory.Internals { MemoryDiagnostics.DecrementTotalUndisposedAllocationCount(); } + else if (this.allocationStackTrace != null) + { + MemoryDiagnostics.RaiseUndisposedMemoryResource(this.allocationStackTrace); + } this.Release(); } diff --git a/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs index 55f14fdfa..5fab655cb 100644 --- a/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/MemoryDiagnosticsTests.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Runtime.CompilerServices; +using System.Threading; using Microsoft.DotNet.RemoteExecutor; using SixLabors.ImageSharp.Diagnostics; using SixLabors.ImageSharp.Memory; @@ -20,60 +21,88 @@ namespace SixLabors.ImageSharp.Tests.Memory.Allocators [Theory] [InlineData(false)] [InlineData(true)] - public void AllocateDispose_Maintains_TotalUndisposedLogicalAllocationCount(bool isGroupOuter) + public void PerfectCleanup_NoLeaksReported(bool isGroupOuter) { RemoteExecutor.Invoke(RunTest, isGroupOuter.ToString()).Dispose(); static void RunTest(string isGroupInner) { bool isGroup = bool.Parse(isGroupInner); + int leakCounter = 0; + MemoryDiagnostics.UndisposedAllocation += _ => Interlocked.Increment(ref leakCounter); + List buffers = new(); - Assert.Equal(0, MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount); + Assert.Equal(0, MemoryDiagnostics.TotalUndisposedAllocationCount); for (int length = 1024; length <= 64 * OneMb; length *= 2) { - long cntBefore = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + long cntBefore = MemoryDiagnostics.TotalUndisposedAllocationCount; IDisposable buffer = isGroup ? Allocator.AllocateGroup(length, 1024) : Allocator.Allocate(length); buffers.Add(buffer); - long cntAfter = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + long cntAfter = MemoryDiagnostics.TotalUndisposedAllocationCount; Assert.True(cntAfter > cntBefore); } foreach (IDisposable buffer in buffers) { - long cntBefore = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + long cntBefore = MemoryDiagnostics.TotalUndisposedAllocationCount; buffer.Dispose(); - long cntAfter = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + long cntAfter = MemoryDiagnostics.TotalUndisposedAllocationCount; Assert.True(cntAfter < cntBefore); } - Assert.Equal(0, MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount); + Assert.Equal(0, MemoryDiagnostics.TotalUndisposedAllocationCount); + Assert.Equal(0, leakCounter); } } [Theory] - [InlineData(false)] - [InlineData(true)] - public void BufferAndGroupFinalizer_DoesNotReduce_TotalUndisposedLogicalAllocationCount(bool isGroupOuter) + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public void MissingCleanup_LeaksAreReported(bool isGroupOuter, bool subscribeLeakHandleOuter) { - RemoteExecutor.Invoke(RunTest, isGroupOuter.ToString()).Dispose(); + RemoteExecutor.Invoke(RunTest, isGroupOuter.ToString(), subscribeLeakHandleOuter.ToString()).Dispose(); - static void RunTest(string isGroupInner) + static void RunTest(string isGroupInner, string subscribeLeakHandleInner) { bool isGroup = bool.Parse(isGroupInner); - Assert.Equal(0, MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount); + bool subscribeLeakHandle = bool.Parse(subscribeLeakHandleInner); + int leakCounter = 0; + bool stackTraceOk = true; + if (subscribeLeakHandle) + { + MemoryDiagnostics.UndisposedAllocation += stackTrace => + { + Interlocked.Increment(ref leakCounter); + stackTraceOk &= stackTrace.Contains(nameof(RunTest)) && stackTrace.Contains(nameof(AllocateAndForget)); + Assert.Contains(nameof(AllocateAndForget), stackTrace); + }; + } + + Assert.Equal(0, MemoryDiagnostics.TotalUndisposedAllocationCount); for (int length = 1024; length <= 64 * OneMb; length *= 2) { - long cntBefore = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + long cntBefore = MemoryDiagnostics.TotalUndisposedAllocationCount; AllocateAndForget(length, isGroup); GC.Collect(); GC.WaitForPendingFinalizers(); GC.Collect(); - long cntAfter = MemoryDiagnostics.GetMemoryInfo().TotalUndisposedAllocationCount; + long cntAfter = MemoryDiagnostics.TotalUndisposedAllocationCount; Assert.True(cntAfter > cntBefore); } + + if (subscribeLeakHandle) + { + // Make sure at least some of the leak callbacks have time to complete on the ThreadPool + Thread.Sleep(200); + Assert.True(leakCounter > 3, $"leakCounter did not count enough leaks ({leakCounter} only)"); + } + + Assert.True(stackTraceOk); } [MethodImpl(MethodImplOptions.NoInlining)] From fb3bd925807073f6b85d1fec942cb2a3392e1ccf Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Tue, 1 Feb 2022 22:46:30 +0100 Subject: [PATCH 4/8] volatile read on totalUndisposedAllocationCount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Günther Foidl --- src/ImageSharp/Diagnostics/MemoryDiagnostics.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index 6bd81d7e5..7ab076e37 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -46,7 +46,7 @@ namespace SixLabors.ImageSharp.Diagnostics /// public static int TotalUndisposedAllocationCount => totalUndisposedAllocationCount; - internal static bool MemoryResourceLeakedSubscribed => undisposedMemoryResourceSubscriptionCounter > 0; + internal static bool MemoryResourceLeakedSubscribed => Volatile.Read(ref undisposedMemoryResourceSubscriptionCounter) > 0; internal static void IncrementTotalUndisposedAllocationCount() => Interlocked.Increment(ref totalUndisposedAllocationCount); From 976b2a89789449cc82791934cab7e673b36dff10 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Tue, 1 Feb 2022 22:48:49 +0100 Subject: [PATCH 5/8] review suggestions --- .../Diagnostics/MemoryDiagnostics.cs | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index 6bd81d7e5..1cd72152b 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -20,6 +20,7 @@ namespace SixLabors.ImageSharp.Diagnostics private static UndisposedMemoryResourceDelegate undisposedMemoryResource; private static int undisposedMemoryResourceSubscriptionCounter; + private static readonly object SyncRoot = new(); /// /// Fires when an ImageSharp object's undisposed memory resource leaks to the finalizer. @@ -30,14 +31,20 @@ namespace SixLabors.ImageSharp.Diagnostics { add { - Interlocked.Increment(ref undisposedMemoryResourceSubscriptionCounter); - undisposedMemoryResource += value; + lock (SyncRoot) + { + undisposedMemoryResourceSubscriptionCounter++; + undisposedMemoryResource += value; + } } remove { - undisposedMemoryResource -= value; - Interlocked.Decrement(ref undisposedMemoryResourceSubscriptionCounter); + lock (SyncRoot) + { + undisposedMemoryResource -= value; + undisposedMemoryResourceSubscriptionCounter--; + } } } @@ -56,6 +63,11 @@ namespace SixLabors.ImageSharp.Diagnostics internal static void RaiseUndisposedMemoryResource(string allocationStackTrace) { + if (undisposedMemoryResource is null) + { + return; + } + // Schedule on the ThreadPool, to avoid user callback messing up the finalizer thread. ThreadPool.QueueUserWorkItem(_ => undisposedMemoryResource?.Invoke(allocationStackTrace)); } From 60108df2c5b9a1b6e4c9c698d52e9b1370b0d40c Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Tue, 1 Feb 2022 22:51:09 +0100 Subject: [PATCH 6/8] consolidate naming --- .../Diagnostics/MemoryDiagnostics.cs | 25 +++++++++++-------- .../RefCountedMemoryLifetimeGuard.cs | 2 +- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index e22e0516c..5c4e1aea4 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -9,7 +9,7 @@ namespace SixLabors.ImageSharp.Diagnostics /// /// Represents the method to handle . /// - public delegate void UndisposedMemoryResourceDelegate(string allocationStackTrace); + public delegate void UndisposedAllocationDelegate(string allocationStackTrace); /// /// Utilities to track memory usage and detect memory leaks from not disposing ImageSharp objects. @@ -18,8 +18,8 @@ namespace SixLabors.ImageSharp.Diagnostics { private static int totalUndisposedAllocationCount; - private static UndisposedMemoryResourceDelegate undisposedMemoryResource; - private static int undisposedMemoryResourceSubscriptionCounter; + private static UndisposedAllocationDelegate undisposedAllocation; + private static int undisposedAllocationSubscriptionCounter; private static readonly object SyncRoot = new(); /// @@ -27,14 +27,14 @@ namespace SixLabors.ImageSharp.Diagnostics /// The event brings significant overhead, and is intended to be used for troubleshooting only. /// For production diagnostics, use . /// - public static event UndisposedMemoryResourceDelegate UndisposedAllocation + public static event UndisposedAllocationDelegate UndisposedAllocation { add { lock (SyncRoot) { - undisposedMemoryResourceSubscriptionCounter++; - undisposedMemoryResource += value; + undisposedAllocationSubscriptionCounter++; + undisposedAllocation += value; } } @@ -42,8 +42,8 @@ namespace SixLabors.ImageSharp.Diagnostics { lock (SyncRoot) { - undisposedMemoryResource -= value; - undisposedMemoryResourceSubscriptionCounter--; + undisposedAllocation -= value; + undisposedAllocationSubscriptionCounter--; } } } @@ -53,7 +53,7 @@ namespace SixLabors.ImageSharp.Diagnostics /// public static int TotalUndisposedAllocationCount => totalUndisposedAllocationCount; - internal static bool MemoryResourceLeakedSubscribed => Volatile.Read(ref undisposedMemoryResourceSubscriptionCounter) > 0; + internal static bool UndisposedAllocationSubscribed => Volatile.Read(ref undisposedAllocationSubscriptionCounter) > 0; internal static void IncrementTotalUndisposedAllocationCount() => Interlocked.Increment(ref totalUndisposedAllocationCount); @@ -63,13 +63,16 @@ namespace SixLabors.ImageSharp.Diagnostics internal static void RaiseUndisposedMemoryResource(string allocationStackTrace) { - if (undisposedMemoryResource is null) + if (undisposedAllocation is null) { return; } // Schedule on the ThreadPool, to avoid user callback messing up the finalizer thread. - ThreadPool.QueueUserWorkItem(_ => undisposedMemoryResource?.Invoke(allocationStackTrace)); + ThreadPool.QueueUserWorkItem( + stackTrace => undisposedAllocation?.Invoke(stackTrace), + allocationStackTrace, + preferLocal: true); } } } diff --git a/src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs b/src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs index 333ef85c2..1c7d6cdfa 100644 --- a/src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs +++ b/src/ImageSharp/Memory/Allocators/Internals/RefCountedMemoryLifetimeGuard.cs @@ -21,7 +21,7 @@ namespace SixLabors.ImageSharp.Memory.Internals protected RefCountedMemoryLifetimeGuard() { - if (MemoryDiagnostics.MemoryResourceLeakedSubscribed) + if (MemoryDiagnostics.UndisposedAllocationSubscribed) { this.allocationStackTrace = Environment.StackTrace; } From cf288f10bbcbe4ba3e32bb9b89426bd1f403663b Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Wed, 2 Feb 2022 13:38:06 +0100 Subject: [PATCH 7/8] preferLocal: false MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Günther Foidl --- src/ImageSharp/Diagnostics/MemoryDiagnostics.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index 5c4e1aea4..7ea484175 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -72,7 +72,7 @@ namespace SixLabors.ImageSharp.Diagnostics ThreadPool.QueueUserWorkItem( stackTrace => undisposedAllocation?.Invoke(stackTrace), allocationStackTrace, - preferLocal: true); + preferLocal: false); } } } From c38825ba2a793dc7ea238fddec2afb9ed1787aa1 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Thu, 3 Feb 2022 23:05:28 +1100 Subject: [PATCH 8/8] Fix for .NET FX --- src/ImageSharp/Diagnostics/MemoryDiagnostics.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index 7ea484175..d83e737f1 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -1,7 +1,6 @@ -// Copyright (c) Six Labors. +// Copyright (c) Six Labors. // Licensed under the Apache License, Version 2.0. -using System; using System.Threading; namespace SixLabors.ImageSharp.Diagnostics @@ -69,10 +68,16 @@ namespace SixLabors.ImageSharp.Diagnostics } // 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); +#else + ThreadPool.QueueUserWorkItem( + stackTrace => undisposedAllocation?.Invoke((string)stackTrace), + allocationStackTrace); +#endif } } }