Repository navigation
Catch up generated files - #3078
Conversation
mdboom
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⬇️ Low Change: Bug fix Merge Risk: ⚪ Minimal · up to This updates generated CUDA bindings so nested anonymous types map to the real header types. Wrapper objects now track memory ownership explicitly, which avoids uncollectable reference cycles. No concrete defects were identified, and the change looks ready to merge.
Comment |
brandon-b-miller
left a comment
brandon-b-miller
left a comment
There was a problem hiding this comment.
LGTM.
Since this is mostly generated code, to review I tried to find a place where this claim failed to hold:
does not actually have any runtime impact
So I measured before/after this change vs main and found no changes
cuda_bindings_driver__anon_pod17 base_sizeof=128 head_sizeof=128 same
cuda_bindings_runtime__anon_pod5 base_sizeof=56 head_sizeof=56 same
...
0 of 59 expressions change size
|
CI Errors look like a flake (503) |
|
2 participants
This catches up the generated cuda_bindings files with two changes:
!546: Handle nesting of unions and structs correctly. This affects how these structs are declared (making more constructions possible) but does not actually have any runtime impact.
!586: Handle cyclical garbage correctly. An agent rightfully identified that reference cycles between automatically generated classes would never be collected. Unfortunately, Cython does not offer a way to handle this correctly and optimally (no way to override
tp_visitandtp_traverse), so I've had to revert back to the old approach of an extra boolean flag to indicate whether we own the memory pointed to (rather than usingNULLfor this). This is a latent bug -- no known uses of it in the wild.