-
Notifications
You must be signed in to change notification settings - Fork 30
Leak testing #490
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
base: main
Are you sure you want to change the base?
Leak testing #490
Conversation
ccummingsNV
commented
Sep 5, 2025
- If live object tracking is enabled, uses it to detect tests that leak objects
- Adds a marker that says a test is known to leak, and what it leaks
skallweitNV
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly fine.
I don't like the new report_live_objects API. I also think we don't need the to_string function on the LiveObjectInfo struct as this really just serves the purpose of populating a python dictionary.
Then I would really welcome if we can move all the leak tracking code into a new leak_tracking.py file or similar, just leaving the hooks in plugin to call into the other module. This will make it much easier to separate it out again if need be.
| #if SGL_ENABLE_OBJECT_TRACKING | ||
| /// Reports current set of live objects. | ||
| static void report_live_objects(); | ||
| static std::vector<LiveObjectInfo> report_live_objects(bool log_to_tty = true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this API too much, mostly because we have similar calls in slang-rhi and D3D12. Let's leave report_live_objects as is (printing to stdout).
Can we add something like get_live_object_infos() to return the list?
|
|
||
| [tool.pytest.ini_options] | ||
| pythonpath = ["."] | ||
| markers = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this definition to the slangpy pytest plugin somehow?
| std::string LiveObjectInfo::to_string() | ||
| { | ||
| return fmt::format( | ||
| "address={}, self_py={}, ref_count={}, class_name={}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these to_string functions usually return something including the class name LiveObjectInfo(...)
| # Types to ignore when checking for leaked objects | ||
| # - The reflection types are created and cached per device when buffers are loaded, so are hard | ||
| # to identify as actual leaks. | ||
| # - CoopVec is created on demand within the device when the coopvec api is used, and so will appear |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # - CoopVec is created on demand within the device when the coopvec api is used, and so will appear | |
| # - The reflection types are created and cached per device when buffers are loaded, so are hard | |
| # to identify as actual leaks. | |
| # - CoopVec is created on demand within the device when the coopvec API is used, and so will appear | |
| # as a leak for cached devices. | |
| IGNORE_LIVE_OBJECT_TYPES = ["NativeSlangType", "TypeLayoutReflection", "TypeReflection", "CoopVec"] | |
| raise RuntimeError("Unsupported platform") | ||
|
|
||
| # If live object tracking is supported, enable leak tracking | ||
| LEAK_TRACKING_ENABLED = hasattr(spy.Object, "report_live_objects") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move all the leak checking specifics into a new leak_tracking.py module?
|
@ccummingsNV Is this still being worked on, or should it be moved to draft? |