Fix build warnings and optimize handle passing #1526
Merged
+63
−63
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Fix Cython nogil placement: move nogil after except+ (44 warnings) - Fix unreachable code in ManagedMemoryResource (move super().__init__ inside CUDA 13+ conditional block) - Optimize C++ functions to accept handles by const& instead of by value, avoiding unnecessary atomic ref count operations on shared_ptr
Andy-Jost
added
enhancement
Contributor
Contributor
Author
|
/ok to test 6686b20 |
This comment has been minimized.
This comment has been minimized.
rparolin
reviewed
Jan 24, 2026
| cdef ContextHandle create_context_handle_ref(cydriver.CUcontext ctx) nogil except+ | ||
| cdef ContextHandle get_primary_context(int device_id) nogil except+ | ||
| cdef ContextHandle get_current_context() nogil except+ | ||
| cdef ContextHandle create_context_handle_ref(cydriver.CUcontext ctx) except+ nogil |
Collaborator
There was a problem hiding this comment.
Can you include in the PR description why nogil except+ is different compared to except+ nogil?
rparolin
approved these changes
Jan 24, 2026
Collaborator
rparolin
left a comment
rparolin
left a comment
There was a problem hiding this comment.
lgtm, just add a single comment asking for clarification on a fix.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
nogilplacement (should appear afterexcept+; no functional difference, just clears a warning saying this will be an error in future version of Cython)ManagedMemoryResourceconst&to avoid atomic ref count operationsChanges
Cython
nogilplacement (_resource_handles.pxd,_resource_handles.pyx):func(...) nogil except+tofunc(...) except+ nogilDead code fix (
_managed_memory_resource.pyx):super().__init__()inside theIF CUDA_CORE_BUILD_MAJOR >= 13blockELSEbranch raises an exception, making code after it unreachableC++ const& optimization (
.hpp,.cpp,.pxd,.pyx):shared_ptrhandles now take them byconst&create_stream_handle,create_event_handle,deviceptr_alloc_from_pool,deviceptr_alloc_async,deviceptr_import_ipc,set_deallocation_streamTest plan