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

add SKNativeObject allocation logging #2157

Closed
wants to merge 5 commits into from
Closed

Conversation

mgood7123
Copy link
Contributor

@mgood7123 mgood7123 commented Jul 7, 2022

Description of Change

can be toggled on/off via SKNativeObject.LOG_ALLOCATIONS (default is false)

Bugs Fixed

None.

API Changes

None.

Behavioral Changes

None.

Required skia PR

#2157

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Merged related skia PRs
  • Changes adhere to coding standard
  • Updated documentation

Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. We should add some logging, but this must be a Debug-only feature, internal and opt-in by setting some property.

If you have a look at the way I set up THROW_OBJECT_EXCEPTIONS. It is a define that was added in debug builds:

The reason for internal is to avoid any accidental APIS leaking, and then when it is in the conditions, we can do checks with and without the logging - maybe for perf or to determine things are working correctly for tests.

@mgood7123
Copy link
Contributor Author

mgood7123 commented Aug 17, 2022

Thanks for this PR. We should add some logging, but this must be a Debug-only feature, internal and opt-in by setting some property.

If you have a look at the way I set up THROW_OBJECT_EXCEPTIONS. It is a define that was added in debug builds:

* https://github.com/mono/SkiaSharp/blob/cb424e5230471ed300319e97cc7cb4fd9f771e03/source/SkiaSharp.Build.props#L88

* https://github.com/mono/SkiaSharp/blob/e603133c3548bbd3835cf1012eac80ce5640f209/binding/Binding/HandleDictionary.cs#L16

The reason for internal is to avoid any accidental APIS leaking, and then when it is in the conditions, we can do checks with and without the logging - maybe for perf or to determine things are working correctly for tests.

hmmmm i still like that it is toggle-able and public, as it may help end users track down allocations that they forget to dispose (which will add memory pressure until GC occurs)

making it togglable via building skiasharp itself (and via debug build) would likely be combersome since the end user will likely be debug building their app and not expect to build a debug build of skiasharp just to enable allocation logging

maybe we could offer both a debug nuget and a release nuget so that the end user does not need to manually build skiasharp as debug?

this way, we still keep the API's but only expose them in the debug build of skiasharp (assumable via #if DEBUG)

@mgood7123 mgood7123 changed the title add SKNativeObject allocation tracking add SKNativeObject allocation logging Aug 21, 2022
@mattleibow
Copy link
Contributor

Closing this one as I am not sure this is the best way. Some more design/discussion/research needs to be done to make use of the tracing features in the .NET SDK: #2777

@mattleibow mattleibow closed this Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants