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.

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

Basic memory diagnostics #1883

Closed
antonfirsov opened this issue Dec 9, 2021 · 4 comments · Fixed by #1969
Closed

Basic memory diagnostics #1883

antonfirsov opened this issue Dec 9, 2021 · 4 comments · Fixed by #1969
Labels

Comments

@antonfirsov
Copy link
Member

Spinning off #1739 (comment) to an issue.

We need expose some basic memory diagnostic utilities from library code:

namespace SixLabors.ImageSharp.Diagnostics
{
    public static class MemoryTracker
    {
        // Count outstanding MemoryAllocator allocations. Dispose reduces the counter, finalizer doesn't.
        public static long TotalUndisposedLogicalAllocationCount { get; }

        // Make the library throw in finalizers if stuff was left undisposed
        public static bool EnableStrictDisposeWatcher { get; set; }
    }
}

We should also introduce standard event counters, but that's a different story for a separate issue later.

@antonfirsov antonfirsov added the API label Dec 9, 2021
@antonfirsov antonfirsov added this to the 2.0.0 milestone Dec 9, 2021
@gfoidl
Copy link
Contributor

gfoidl commented Dec 12, 2021

Instead of individual static properties for "memory info" there should be only static method, that in turn return a type which represents the memory info.

namespace SixLabors.ImageSharp.Diagnostics
{
    public static class MemoryTracker
    {
        public static MemoryInfo GetMemoryInfo();

        // Make the library throw in finalizers if stuff was left undisposed
        public static bool EnableStrictDisposeWatcher { get; set; }
    }

    // or a normal struct, here just for simplicity
    public readonly record struct MemoryInfo(long TotalUndisposedLogicalAllocationCount);
}

That way it's easier extend the memory info and/or get the "info at once" without the need to query several properties (in the assumption that there will be more properties for memory info in the future).

This should be the first point to start with.
Based on top of that, event counters (respectively an event source that provides counters) can be created.


Personally I'm quite busy now, so I don't feel able to do the first step. But (later) for the event counters I'm sure to help out (use these in my work project quite extensively, so some good patterns evolved for implementing).

@antonfirsov
Copy link
Member Author

@gfoidl great idea thanks!

But (later) for the event counters I'm sure to help out

We would definitely appreciate help!

@antonfirsov
Copy link
Member Author

In general, telemetry is something that would be great to see being introduced in the library, but we are struggling to find time for it.

@antonfirsov
Copy link
Member Author

While documenting the implementation I realized I'm not sure about the MemoryInfo variant anymore. This is not GC, I don't see us having nearly as many statistics as GCMemoryInfo, at least not valuable ones.

A single static property is much easier to discover. I prefer whatever helps the user more at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants