Memory allocation allocation tracker test helper #2082

tocsoft · 2022-04-09T21:36:23Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

This adds some new test helpers APIs + updates the MemoryDiagnostics class to consistently track the memory buffers per test.

Also adds 2 helpers in to the tests

ValidateDisposedMemoryAllocationsAttribute - This attributes a can be applied to test classes and Fact/Theorys directly to automatically apply the memory application tracking and validation.
MemoryAllocatorValidator static class can be used in places the attribute is not appropriate, i.e. inside Remote Executors.

I've also applied some fixes targeting the Decoder tests

this includes the fix from #2081 (which I will merge into this PR so we don't loose attribute of the fix) if we decide this approach is suitable...otherwise we should just merge that one.

tocsoft · 2022-04-09T21:38:24Z

@antonfirsov It would be useful if you could confirm I'm not doing anything too critical/stupid fiddling with the MemoryDiagnostics class.

br3aker · 2022-04-10T08:20:33Z

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

+            catch
+            {
+                this.Frame?.Dispose();
+                throw;
+            }


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

ImageSharp/src/ImageSharp/Formats/Jpeg/JpegDecoder.cs

Lines 19 to 26 in 136cc14

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);

}

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

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

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

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.

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

JimBobSquarePants

Ah this is fantastic! Exactly what we need. 😍

br3aker

ValidateDisposedMemoryAllocationsAttribute is a blessing, also cool that current coverage reports no leaks 😎

tocsoft · 2022-04-10T12:06:37Z

ValidateDisposedMemoryAllocationsAttribute is a blessing, also cool that current coverage reports no leaks 😎

no leaks (anymore) in the few tests I added it to anyway :D

There will be a need to follow up adding this to a lot more of the test... I didn't want to blow out this PR fixing leaks hiding the framework changes.

Despite this being approved I think its worth leave this one open a bit longer before merging to give anyone with more knowledge on how my changed to the MemoryDiagnostics class might impact perf.

codecov · 2022-04-11T11:09:15Z

Codecov Report

Merging #2082 (136cc14) into main (136cc14) will not change coverage.
The diff coverage is n/a.

❗ Current head 136cc14 differs from pull request most recent head 558ff99. Consider uploading reports for the commit 558ff99 to get more accurate results

@@          Coverage Diff          @@
##            main   #2082   +/-   ##
=====================================
  Coverage     88%     88%           
=====================================
  Files        990     990           
  Lines      52619   52619           
  Branches    6617    6617           
=====================================
  Hits       46443   46443           
  Misses      5083    5083           
  Partials    1093    1093

Flag	Coverage Δ
unittests	`88% <0%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 136cc14...558ff99. Read the comment docs.

antonfirsov

We need to separate the AsyncLocal test-scoped validation from the global MemoryDiagnostics.TotalUndisposedAllocationCount counter.

antonfirsov · 2022-04-11T12:15:02Z

src/ImageSharp/Diagnostics/MemoryDiagnostics.cs

        /// <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;


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.

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.

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.

antonfirsov · 2022-04-11T12:23:56Z

src/ImageSharp/Formats/Tiff/Compression/Decompressors/JpegTiffCompression.cs

+                        using var decompressedBuffer = spectralConverterGray.GetPixelBuffer(CancellationToken.None);
+                        CopyImageBytesToBuffer(buffer, decompressedBuffer);


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

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

antonfirsov · 2022-04-11T12:27:05Z

tests/ImageSharp.Tests/MemoryAllocatorValidator.cs

+{
+    public static class MemoryAllocatorValidator
+    {
+        public static IDisposable MonitorAllocations(int max = 0)


Suggested change

public static IDisposable MonitorAllocations(int max = 0)

public static TestMemoryAllocatorDisposable MonitorAllocations(int max = 0)

No point for TestMemoryAllocatorDisposable being a struct, if we box it into IDisposable in the end.

antonfirsov · 2022-04-11T12:30:48Z

tests/ImageSharp.Tests/MemoryAllocatorValidator.cs

+            return new TestMemoryAllocatorDisposable(max);
+        }
+
+        public static void ValidateAllocation(int max = 0)


Is there a use case for max > 0? Do we really want to allow any test having leak-validation enabled to leak some memory? Or am I misunderstanding the purpose?

no really I can't remember why I added it really... it was probably a bad idea really, consider it gone.

antonfirsov · 2022-04-11T13:13:12Z

What I would do is to expose (for now internal) BufferAllocated / BufferDestroyed events and do something like:

internal static void IncrementTotalUndisposedAllocationCount()
{
    Interlocked.Increment(ref totalUndisposedAllocationCount);
    bufferAllocatedBackingDelegate?.Invoke();
}

Then move the AsyncLocal utility to test code and control it by subscribing to the events above.

tocsoft · 2022-04-11T13:45:29Z

What I would do is to expose (for now internal) BufferAllocated / BufferDestroyed events and do something like:
internal static void IncrementTotalUndisposedAllocationCount()
{
    Interlocked.Increment(ref totalUndisposedAllocationCount);
    bufferAllocatedBackingDelegate?.Invoke();
}
Then move the AsyncLocal utility to test code and control it by subscribing to the events above.

I think I like that better its less impactful on the prod code.... i'll have a play and see how feels.

antonfirsov

Much better this way I think, thanks! Two minor remarks.

antonfirsov · 2022-04-11T19:59:55Z

tests/ImageSharp.Tests/MemoryAllocatorValidator.cs

+
+        public class TestMemoryDiagnostics : IDisposable
+        {
+            public int TotalAllocated { get; set; }


This seems to be unused.

I was pre-empting being useful for tests that want to track/verify total number of allocated buffers not just retained buffers... but your right currently not being used.

I'm generally reluctant to add unused/untested features (= stuff to understand, remember and maintain) even to test utility code. We can easily add them later when actual need appears.

But it's really a nit, so I'm not going to block the PR on these two points.

antonfirsov · 2022-04-11T20:01:23Z

tests/ImageSharp.Tests/MemoryAllocatorValidator.cs

+
+            public int TotalRemainingAllocated { get; set; }
+
+            public void Validate(int expectedAllocationCount)


expectedAllocationCount seems to be always 0.

In our current tests your are right, but I was envisioning use cases where we might want to want to validate before all buffers are returned and this is to allow for testing a specific/expected number of buffers are currently allocated still.

tocsoft added 6 commits April 9, 2022 20:20

xunit helper to track undisposed memory

77bb287

revert loader change

3872c60

check disposed buffers for decoders

af7d84a

dispose the next chunk

1557bae

ensure we dispose beffers when decoders throw

2a1ae5c

drop commented code

a30c468

tocsoft added 2 commits April 9, 2022 22:39

stylecopped

399d71d

indent for full framwork

d9af239

tocsoft mentioned this pull request Apr 10, 2022

Jpeg decoder can lead to finalizer memory leaks with specific malformed images #2078

Closed

4 tasks

br3aker reviewed Apr 10, 2022

View reviewed changes

tocsoft added 2 commits April 10, 2022 09:32

Async local doesn't actually work for the finalizers, revert that piece.

99d1266

revert Frame Dispose

558ff99

tocsoft mentioned this pull request Apr 10, 2022

Track memory allocations in tests SixLabors/ImageSharp.Drawing#216

Merged

4 tasks

JimBobSquarePants approved these changes Apr 10, 2022

View reviewed changes

br3aker approved these changes Apr 10, 2022

View reviewed changes

antonfirsov requested changes Apr 11, 2022

View reviewed changes

tocsoft added 2 commits April 11, 2022 16:20

move AsyncLocal out of core lib and into testing

cd7c91c

fix comment

f597507

antonfirsov reviewed Apr 11, 2022

View reviewed changes

antonfirsov approved these changes Apr 12, 2022

View reviewed changes

tocsoft and others added 2 commits April 12, 2022 17:56

revert to allow easier merge

f4f521c

Fix chunk data memory leak when decoding PNG. Fix #2080

03d0b63

tocsoft merged commit 7db4792 into main Apr 12, 2022

tocsoft deleted the sw/allocation-tracker branch April 12, 2022 17:28

tocsoft mentioned this pull request Apr 12, 2022

Fix chunk data memory leak when decoding PNG #2081

Closed

4 tasks

JimBobSquarePants added bug unit tests labels Apr 13, 2022

JimBobSquarePants added this to the 2.1.1 milestone Apr 13, 2022

This was referenced Apr 21, 2022

Update dependency SixLabors.ImageSharp to v2.1.3 - autoclosed RononDex/Astrobot#85

Closed

chore: Update dependency SixLabors.ImageSharp to v2.1.3 spectreconsole/spectre.console#805

Closed

renovate bot mentioned this pull request Apr 28, 2022

chore(deps): update dependency sixlabors.imagesharp to v2.1.1 FantasticFiasco/export-image-dotnet-core#33

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory allocation allocation tracker test helper #2082

Memory allocation allocation tracker test helper #2082

tocsoft commented Apr 9, 2022

tocsoft commented Apr 9, 2022

br3aker Apr 10, 2022

tocsoft Apr 10, 2022

br3aker Apr 10, 2022

JimBobSquarePants Apr 10, 2022

br3aker Apr 10, 2022

JimBobSquarePants Apr 10, 2022

JimBobSquarePants left a comment

br3aker left a comment

tocsoft commented Apr 10, 2022

codecov bot commented Apr 11, 2022 •

edited

antonfirsov left a comment

antonfirsov Apr 11, 2022

tocsoft Apr 11, 2022

antonfirsov Apr 11, 2022

antonfirsov Apr 11, 2022

tocsoft Apr 11, 2022

antonfirsov Apr 11, 2022

antonfirsov Apr 11, 2022 •

edited

tocsoft Apr 11, 2022

antonfirsov commented Apr 11, 2022

tocsoft commented Apr 11, 2022

antonfirsov left a comment

antonfirsov Apr 11, 2022

tocsoft Apr 11, 2022

antonfirsov Apr 12, 2022 •

edited

antonfirsov Apr 11, 2022

tocsoft Apr 11, 2022

	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);
	}

		using var decompressedBuffer = spectralConverterGray.GetPixelBuffer(CancellationToken.None);
		CopyImageBytesToBuffer(buffer, decompressedBuffer);

	public static IDisposable MonitorAllocations(int max = 0)
	public static TestMemoryAllocatorDisposable MonitorAllocations(int max = 0)


		public int TotalRemainingAllocated { get; set; }

		public void Validate(int expectedAllocationCount)

Memory allocation allocation tracker test helper #2082

Memory allocation allocation tracker test helper #2082

Conversation

tocsoft commented Apr 9, 2022

Prerequisites

Description

tocsoft commented Apr 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants left a comment

Choose a reason for hiding this comment

br3aker left a comment

Choose a reason for hiding this comment

tocsoft commented Apr 10, 2022

codecov bot commented Apr 11, 2022 • edited

Codecov Report

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Apr 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov commented Apr 11, 2022

tocsoft commented Apr 11, 2022

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Apr 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 11, 2022 •

edited

antonfirsov Apr 11, 2022 •

edited

antonfirsov Apr 12, 2022 •

edited