Fix #449: Delay construction of Python attributes #847
Conversation
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
|
/ok to test |
| @@ -51,7 +50,7 @@ cdef class StridedMemoryView: | |||
| Pointer to the tensor buffer (as a Python `int`). | |||
| shape : tuple | |||
| Shape of the tensor. | |||
| strides : tuple | |||
| strides : Optional[tuple] | |||
There was a problem hiding this comment.
Just updating docs to match current behavior (not a change in behavior)
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the StridedMemoryView class by delaying the construction of Python attributes (shape, strides, and dtype) until they are actually accessed, resulting in an 18% performance improvement in object creation. The changes convert attributes to Cython cdefs and implement lazy property access.
Key changes:
- Convert
StridedMemoryViewattributes to lazy properties with delayed construction - Add Cython cdef declarations for better performance
- Include comprehensive test coverage for the
__cuda_array_interface__code path
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cuda_core/cuda/core/experimental/_memoryview.pyx | Implements lazy property construction and Cython optimization for StridedMemoryView |
| cuda_core/tests/test_utils.py | Adds test coverage for the CUDA Array Interface code path |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Co-authored-by: Copilot <[email protected]>
|
/ok to test |
leofang
added
enhancement
|
/ok to test |
|
|
||
| # Memoized properties | ||
| cdef tuple _shape | ||
| cdef object _strides |
There was a problem hiding this comment.
IIRC self._strides = None is allowed in Cython even if _strides is typed to tuple, so this might help readability (and perf too?)
| cdef object _strides | |
| cdef tuple _strides |
| @property | ||
| def strides(self) -> Optional[tuple[int]]: | ||
| cdef int itemsize | ||
| if self._strides is None and self.exporting_obj is not None: |
There was a problem hiding this comment.
I see a minor potential concern here. Unlike _shape where we can use None to indicate it's uninitialized, _strides cannot do this because None is a valid value (indicating C-contiguity) which was meant to be a fast path. Now with this check, we turn the fast path into a slow path because we keep re-running the checks (and resetting _strides to None) every time this property is requested. A dumb fix would be to add another boolean member to indicate if _strides is initialized. Thoughts?
Description
For the
StridedMemoryView, this delays construction of the Python typesshape,stridesanddtypeand converts the other simpler objects to regular Cython cdefs.This has an approximately 18% speedup on
StridedMemoryViewobject creation:This also adds tests to cover the
__cuda_array_interface__code path.closes
Checklist