Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add basic memory diagnostic API #1969

Merged
merged 12 commits into from Feb 3, 2022
Merged

Add basic memory diagnostic API #1969

merged 12 commits into from Feb 3, 2022

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Feb 1, 2022 •
edited

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

Resolves #1883. I went with the following API in the end, feels nicer and simpler:

public delegate void UndisposedMemoryResourceDelegate(string allocationStackTrace);

public static class MemoryDiagnostics
{
    // Reports the stack trace of leaked allocations. Subscribe for troubleshooting.
    public static event UndisposedMemoryResourceDelegate UndisposedAllocation;

    public static int TotalUndisposedAllocationCount { get; }
}

JimBobSquarePants
JimBobSquarePants approved these changes Feb 1, 2022
View reviewed changes
Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

Well this is clever!!! 🤲

gfoidl
gfoidl reviewed Feb 1, 2022
View reviewed changes
src/ImageSharp/Diagnostics/MemoryDiagnostics.cs Outdated Show resolved Hide resolved
src/ImageSharp/Diagnostics/MemoryDiagnostics.cs Outdated Show resolved Hide resolved
src/ImageSharp/Diagnostics/MemoryDiagnostics.cs Outdated Show resolved Hide resolved
@antonfirsov
Copy link
Member Author

@gfoidl as always, thanks for the remarks!

JimBobSquarePants
JimBobSquarePants reviewed Feb 3, 2022
View reviewed changes
src/ImageSharp/Diagnostics/MemoryDiagnostics.cs
@@ -69,10 +68,16 @@ internal static void RaiseUndisposedMemoryResource(string allocationStackTrace)
}

// Schedule on the ThreadPool, to avoid user callback messing up the finalizer thread.
#if NETSTANDARD2_1 || NETCOREAPP2_1_OR_GREATER
Copy link
Member

Choose a reason for hiding this comment

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

https://apisof.net/catalog/737fc3e3-23d4-dfab-294f-8c59c58c5852

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

No worries. Thought I'd better wait until you'd seen this before merging. I'll let you do the honors when you're happy.

@antonfirsov antonfirsov merged commit 259db7c into master Feb 3, 2022
@antonfirsov antonfirsov deleted the af/memory-diagnostics branch February 3, 2022 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JimBobSquarePants JimBobSquarePants JimBobSquarePants approved these changes

@gfoidl gfoidl gfoidl left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Basic memory diagnostics
3 participants
@antonfirsov @JimBobSquarePants @gfoidl