issues-2711 Add PyMemoryView by ikrivosheev · Pull Request #2792 · PyO3/pyo3

ikrivosheev · 2022-11-30T18:28:10Z

Closed: #2711.

birkenfeld · 2022-11-30T19:12:14Z

+pyobject_native_type_core!(PyMemoryView<T>, ffi::PyMemoryView_Type, #checkfunction=ffi::PyMemoryView_Check; T);
+
+impl PyMemoryView<u8> {
+    pub fn new<'p>(py: Python<'p>, memory: &mut [u8], is_writable: bool) -> &'p PyMemoryView<u8> {


Unfortunately this API isn't safe - the memory would have to be borrowed for as long as the returned memoryview lives, but that can't be encoded since Python objects can't have associated lifetimes (except for the 'py pseudo-lifetime).

It might be possible to offer it as unsafe fn with appropriate documentation.

Agreed this isn't safe as-is. We technically already have the unsafe fn in from_ptr. It would also be very hard to use correctly, as if you pass this object to other Python code it's extremely likely this memoryview may live longer than you expect.

A safe alternative may be to accept &'static [u8], although that impairs usability a bit.

Maybe the PyMemoryView::from already started below is the thing we should encourage users to use (they can then wrap their buffers in #[pyclass] types which expose buffer access).

davidhewitt

Hello! Sorry for the long delay in reviewing this, I was a bit overloaded before the Christmas holidays.

Thank you for working on this. This is a great start, however we need to adjust some of the APIs a bit before this can be ready for merge. See other comments 🙂

davidhewitt · 2022-12-26T19:46:27Z

+
+/// Represents a Python `memoryview`.
+#[repr(transparent)]
+pub struct PyMemoryView<T>(PyAny, PhantomData<T>);


The choice to make this generic is interesting, and I've been thinking about this further. At the moment none of PyO3's types (other than buffer) are generic. Also PyMemoryView_Check won't check the generic type is correct, which makes it easy for this type to hold memory views with the wrong T.

For these reasons I think it's better if we remove the generic type from this and just have PyMemoryView.

davidhewitt · 2022-12-26T19:48:53Z

+pyobject_native_type_core!(PyMemoryView<T>, ffi::PyMemoryView_Type, #checkfunction=ffi::PyMemoryView_Check; T);
+
+impl PyMemoryView<u8> {
+    pub fn new<'p>(py: Python<'p>, memory: &mut [u8], is_writable: bool) -> &'p PyMemoryView<u8> {


Agreed this isn't safe as-is. We technically already have the unsafe fn in from_ptr. It would also be very hard to use correctly, as if you pass this object to other Python code it's extremely likely this memoryview may live longer than you expect.

A safe alternative may be to accept &'static [u8], although that impairs usability a bit.

Maybe the PyMemoryView::from already started below is the thing we should encourage users to use (they can then wrap their buffers in #[pyclass] types which expose buffer access).

davidhewitt · 2022-12-26T19:55:45Z

+    /// This function dereferences the raw pointer `ptr` as the
+    /// leading pointer of a slice of length `len`. [As with
+    /// `std::slice::from_raw_parts`, this is
+    /// unsafe](https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html#safety).


Also needs to have a safety note requiring that the created memory view does not outlive the data in ptr. This is extremely hard to use safely!

davidhewitt · 2022-12-26T19:56:29Z

+
+    /// Creates a new Python `memoryview` object from another Python object that
+    /// implements the buffer protocol.
+    pub fn from<'p, I>(py: Python<'p>, src: &'p I) -> PyResult<&'p PyMemoryView<T>>


I might be tempted to just have pub fn from(&PyAny) -> PyResult<&PyMemoryView> to simplify the signature a bit.

davidhewitt · 2022-12-26T19:57:05Z

+    pub fn len(&self) -> usize {
+        unsafe {
+            let size = ffi::Py_SIZE(self.as_ptr());
+            // non-negative Py_ssize_t should always fit into Rust usize
+            size as usize
+        }
+    }


I think this always returns length in u8, not units of T? So again this looks ok, but only with untyped PyMemoryView.

davidhewitt · 2022-12-26T19:57:45Z

+
+    /// Gets the Python string as a byte slice.
+    #[inline]
+    pub fn as_buffer(&self) -> PyResult<PyBuffer<T>> {


... and with untyped PyMemoryView, this can still be generic:

Suggested change

pub fn as_buffer(&self) -> PyResult<PyBuffer<T>> {

pub fn as_buffer<T>(&self) -> PyResult<PyBuffer<T>> {

davidhewitt · 2022-12-26T19:58:50Z

+    {
+        unsafe { py.from_owned_ptr_or_err(ffi::PyMemoryView_FromObject(src.as_ptr())) }
+    }
+


There is possibly also scope to have PyMemoryView::from_buffer which takes PyBuffer as input.

@davidhewitt hmm, I have a question: how I can as_ptr() for PyBuffer?

See https://docs.python.org/3/c-api/memoryview.html#c.PyMemoryView_FromBuffer - no need to .as_ptr() the buffer structure

ikrivosheev · 2023-12-25T16:38:07Z

@birkenfeld @davidhewitt hello! Sorry for the delay... I extended PyMemoryView and added some new methods.

davidhewitt · 2023-12-26T09:10:10Z

+        unsafe { self.downcast_unchecked() }
+    }
+
+    /// Gets the Python string as a byte slice.


Comment here looks out of date.

davidhewitt · 2023-12-26T09:15:38Z

+
+    /// Gets the Python string as a byte slice.
+    #[inline]
+    pub fn as_buffer<T: Element>(&self) -> PyResult<PyBuffer<T>> {


I'm trying to think, is there a memory safety concern casting from bytes to arbitrary elements T here? Potentially as long as the buffer format is checked, it's fine? We require Element: Copy which implies these types are all POD and don't own heap allocations to trigger use-after-free etc

If there is, it was already be present in PyBuffer::get, wasn't it? And that one does indeed check the buffer format using Element::is_compatible_format.

What I would like to see is a unit test to check whether this actually works for anything but PyBuffer<u8>.

davidhewitt · 2023-12-26T09:16:17Z

+    {
+        unsafe { py.from_owned_ptr_or_err(ffi::PyMemoryView_FromObject(src.as_ptr())) }
+    }
+


See https://docs.python.org/3/c-api/memoryview.html#c.PyMemoryView_FromBuffer - no need to .as_ptr() the buffer structure

codspeed-hq · 2024-03-23T14:21:42Z

CodSpeed Performance Report

Merging #2792 will not alter performance

_{Comparing ikrivosheev:feature/issues-2711_py_memory_view (2ed62af) with main (f37c682)}

Summary

✅ 63 untouched benchmarks

davidhewitt · 2024-12-06T10:50:22Z

This has been stale for so long and we have the basic type merged now, will close this and defer possible improvements for a future time.

ikrivosheev mentioned this pull request Nov 30, 2022

add memory view wrapper #2711

Closed

birkenfeld reviewed Nov 30, 2022

View reviewed changes

davidhewitt requested changes Dec 26, 2022

View reviewed changes

adamreichold mentioned this pull request Oct 15, 2023

Add PyMemoryView type #3514

Merged

issues-2711 Add PyMemoryView

2ed62af

ikrivosheev force-pushed the feature/issues-2711_py_memory_view branch from c2d8c96 to 2ed62af Compare December 25, 2023 16:35

davidhewitt reviewed Dec 26, 2023

View reviewed changes

davidhewitt closed this Dec 6, 2024

	pub fn as_buffer(&self) -> PyResult<PyBuffer<T>> {
	pub fn as_buffer<T>(&self) -> PyResult<PyBuffer<T>> {

Conversation

ikrivosheev commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikrivosheev commented Dec 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed-hq Bot commented Mar 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #2792 will not alter performance

Summary

Uh oh!

davidhewitt commented Dec 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ikrivosheev commented Nov 30, 2022 •

edited

Loading

codspeed-hq Bot commented Mar 23, 2024 •

edited

Loading