Reimpl Buffer Protocol and memoryview support ndarray with shape, stride and suboffset by qingshi163 · Pull Request #3340 · RustPython/RustPython

qingshi163 · 2021-10-19T14:34:05Z

There was a misunderstanding about the code for buffer protocol I wrote at beginning, so the related pr #3029 and #3043 finally broke the memoryview(the test did not show). When I review there pr I also just broken the mind, really sorry about that.
But in the pr I reimplement the protocol with more efficient way(thanks for #3121), this should works better.

qingshi163 · 2021-10-19T14:37:19Z

-    #[derive(Debug)]
+    #[pyclass(noattr, module = false, name = "_buffered_raw_buffer")]
+    #[derive(Debug, PyValue)]
    struct BufferedRawBuffer {
        data: PyMutex<Vec<u8>>,
        range: Range<usize>,
    }
-    impl BufferInternal for BufferedRawBuffer {
-        fn obj_bytes(&self) -> BorrowedValue<[u8]> {
-            BorrowedValue::map(self.data.lock().into(), |data| &data[self.range.clone()])
-        }
+    #[pyimpl(with(AsBuffer))]
+    impl BufferedRawBuffer {}

-        fn obj_bytes_mut(&self) -> BorrowedValueMut<[u8]> {
-            BorrowedValueMut::map(self.data.lock().into(), |data| {
-                &mut data[self.range.clone()]
-            })
-        }
+    static BUFFERED_RAW_BUFFER_METHODS: BufferMethods = BufferMethods {


I am trying to implement BufferedRawBuffer as an internal class so we can downcast from pyobjectref when the buffer methods is called. but I am getting panic for 'static type has not been initialized', anything missing here?
@youknowone

I am sorry, I didn't catch I was mentioned here. You added pyclass to BufferredRawBuffer but with noattr. I think a class is initialized only when it is a pyattr. In this case, you can manually initailize it in make_modue. Look for noattr in vm/src/stdlib/sys.rs. There are a few examples about it.
Probably we'd better to fix pymodule macro to initialize them regardless of noattr

qingshi163 · 2021-10-19T14:39:13Z

+    // GUARANTEE: called only if the buffer option is contiguous
+    pub contiguous: Option<fn(&PyObjectRef) -> BorrowedValue<[u8]>>,
+    // GUARANTEE: called only if the buffer option is contiguous
+    pub contiguous_mut: Option<fn(&PyObjectRef) -> BorrowedValueMut<[u8]>>,
+    // collect bytes to buf when buffer option is not contiguous
+    pub collect_bytes: Option<fn(&PyObjectRef, buf: &mut Vec<u8>)>,


These three function should return the bytes that available in the current class. compare to obj_bytes and obj_bytes_mut that should return the whole bytes for the top class.

qingshi163 · 2021-10-25T15:31:18Z

            none_type: singletons::PyNone::init_bare_type().clone(),
            not_implemented_type: singletons::PyNotImplemented::init_bare_type().clone(),
            generic_alias_type: genericalias::PyGenericAlias::init_bare_type().clone(),
+            vec_buffer_type: VecBuffer::init_bare_type().clone(),


I had put the type here, it works, but I don't know does it has better solution?

If you don't need to use it through vm.ctx.types.vec_buffer_type, you don't need to add a member here. static types will be saved for each of their static_class.
(In other word, if you need to use it like that, this is the right place.)

youknowone · 2021-10-25T16:53:55Z

How CPython handle it? Are those python classes there too?

qingshi163 · 2021-10-25T19:39:56Z

How CPython handle it? Are those python classes there too?

CPython is using void* with buffer options for the buffer data, we cannot do like that because we may have the lock before the data, so we have to visit data via function that base on each class.

Secondly, CPython manage the exports by manually call PyBuffer_Release that will call the bf_releasebuffer slots, we are manage it more safer way by wrap it with PyBuffer.

As we are using static functions for what buffer need, our appoarch shouldn't have a lot of overhead.

For contiguous, contiguous_mut and collect_bytes, I think there is a way to rid out it by move some logic from memoryview to PyBuffer, I will work on it push to here.

For VecBuffer, CPython don't have it because it use void pointer, we need type to retrive the data from the python object so I use VecBuffer to wrap a Vec to support as a buffer.

youknowone · 2021-10-26T07:48:20Z

For the polymorphism using void* cases, It sounds more like trait object like &dyn T or *dyn T than a python class to me. Did you also checked this option? (It could be totally wrong. I didn't look in the details.)

qingshi163 · 2021-10-28T15:39:10Z

For the polymorphism using void* cases, It sounds more like trait object like &dyn T or *dyn T than a python class to me. Did you also checked this option? (It could be totally wrong. I didn't look in the details.)

The original design for void* case is to use dyn T(dyn BufferInternal).
The issue for dyn T is,

first, PyBuffer own an obj as PyObjectRef, if we are going to have another field for dyn T, we doubled the rc cost.
second, for us able to get buffer via python object, we are implement BufferInternal for PyRef<T: PyValue>, this is another rc. so finally the field type will be like PyRc<PyRef<dyn BufferInternal>>

so I decide to use static functions and downcast the obj to visit the class. the only issue for it is when we need buffer a rust vec, we need a wrap object that implement PyValue so we can able to downcast it. but I think that is only const cost when initialize and zero cost on the runtime.

qingshi163 · 2021-11-05T07:11:26Z

    pub none_type: PyTypeRef,
    pub not_implemented_type: PyTypeRef,
    pub generic_alias_type: PyTypeRef,
+    pub vec_buffer_type: PyTypeRef,


@youknowone can you help me to move it away?

I attached a commit for this

youknowone · 2021-11-06T06:12:41Z

+    }
+}
+
+pub struct SaturatedSliceIterator {


Rust iterators are typically named Iter (mostly because Iterator is a trait name)

Suggested change

pub struct SaturatedSliceIterator {

pub struct SaturatedSliceIter {

youknowone · 2021-11-06T06:14:32Z

+    pub index: isize,
+    pub step: isize,
+    pub len: usize,


This is fields of iterator, but is it pub?

no, it should not be pub.

youknowone · 2021-11-06T07:08:27Z

-        self.internal.obj_bytes().to_vec()
+    /// # Safety
+    /// assume the buffer is contiguous
+    pub unsafe fn contiguous(&self) -> BorrowedValue<[u8]> {


if we are going to name this under rust convention,

Suggested change

pub unsafe fn contiguous(&self) -> BorrowedValue<[u8]> {

pub unsafe fn contiguous_unchecked(&self) -> BorrowedValue<[u8]> {

youknowone · 2021-11-06T07:11:40Z

+        self.obj_bytes_mut()
+    }
+
+    pub fn collect(&self, buf: &mut Vec<u8>) {


collect typically returns the collection itself, but this is extending existing vector.

Suggested change

pub fn collect(&self, buf: &mut Vec<u8>) {

pub fn append_to(&self, buf: &mut Vec<u8>) {

youknowone · 2021-11-06T07:15:27Z

+
+    /// this function do not check the bound
+    /// panic if indices.len() != ndim
+    pub fn get_position_fast(&self, indices: &[usize]) -> isize {


if the name get_ is not coming from CPython, I want to suggest to remove it because this function is not perform getter or getitem. Just fast_position like fast_getitem?

qingshi163 · 2021-11-07T19:32:41Z

#3244

youknowone · 2021-11-09T15:04:38Z

+        let format_spec = Self::parse_format(&buffer.desc.format, vm)?;
+        let desc = buffer.desc.clone();
+
+        let mut zelf = PyMemoryView {
+            buffer: ManuallyDrop::new(buffer),
            released: AtomicCell::new(false),
-            start: range.start * itemsize,
-            stop: range.end * itemsize,
-            step: 1,
+            start: 0,
            format_spec,
+            desc,
            hash: OnceCell::new(),
-        }
-        .validate())
-    }
-
-    fn as_contiguous(&self) -> Option<BorrowedValue<[u8]>> {
-        if !self.buffer.options.contiguous {
-            return None;
-        }
-        Some(self.obj_bytes())
-    }
+        };


these lines looks same as from_buffer. then will let mut zelf = Self::from_buffer(buffer, vm) works?

youknowone · 2021-11-09T15:19:00Z


-    fn retain(&self) {}
-}
+    fn collect(&self, buf: &mut Vec<u8>) {


here is another collect not returning buffer

youknowone · 2021-11-09T15:25:55Z

@@ -123,17 +147,293 @@ impl TryFromBorrowedObject for PyBuffer {
 // but it is not supported by Rust


the comment looks outdated

youknowone · 2021-11-09T15:32:47Z

+
+#[pyimpl(flags(BASETYPE), with(Constructor))]
+impl VecBuffer {
+    pub fn new(data: Vec<u8>) -> Self {


From is typically implemented for conversion

qingshi163 · 2021-11-13T05:13:25Z

We should have the _test_buffer for multi-dimention buffer test. but I think this pr need to be merge now to fix the existing issue.

qingshi163 commented Oct 19, 2021

View reviewed changes

qingshi163 force-pushed the fix-buffer branch from 88b32ec to 0041a81 Compare October 25, 2021 15:27

qingshi163 commented Oct 25, 2021

View reviewed changes

qingshi163 force-pushed the fix-buffer branch from 0041a81 to aec309c Compare October 25, 2021 19:05

qingshi163 added 11 commits November 3, 2021 20:16

Fix buffer protocol and memoryview

4dccbfb

Fix io and array the right way to use buffer protocol

75d5744

fix rebase

18c7add

fix memoryview and introduce VecBuffer

ee2dacd

fix deadlock

a3c5597

fix memoryview avoid double release buffer

a39b260

impl ndim buffer support

27d67c2

first implement nd array for buffer and memoryview

aef1679

fix slice adjust_indices

ee41499

fix adjust_indices introduct SaturatedSliceIterator

41c3e13

fix memoryview cmp

111cd3b

qingshi163 force-pushed the fix-buffer branch from aec309c to 111cd3b Compare November 3, 2021 18:43

qingshi163 changed the title ~~[WIP] Fix buffer protocol and memoryview~~ [WIP] Reimpl Buffer Protocol and memoryview support ndarray with shape, stride and suboffset Nov 3, 2021

qingshi163 added 5 commits November 4, 2021 19:28

fix adjust_indices positive_order

970c408

fix bug mark passed test

4c9a8bc

fix clippy

072605a

fix set_item_slice deadlock, optimize buffer

7033a7d

Delete @test_113974_tmp

29c9d0e

qingshi163 commented Nov 5, 2021

View reviewed changes

qingshi163 changed the title ~~[WIP] Reimpl Buffer Protocol and memoryview support ndarray with shape, stride and suboffset~~ Reimpl Buffer Protocol and memoryview support ndarray with shape, stride and suboffset Nov 5, 2021

qingshi163 marked this pull request as ready for review November 5, 2021 07:12

qingshi163 requested a review from youknowone November 5, 2021 07:12

youknowone reviewed Nov 6, 2021

View reviewed changes

Remove vec_buffer_type

ffeb638

youknowone force-pushed the fix-buffer branch from 6c6007e to ffeb638 Compare November 6, 2021 07:17

qingshi163 added 2 commits November 6, 2021 18:25

rusty names

3d72bde

impl memoryview multi index

43cf696

qingshi163 requested a review from youknowone November 8, 2021 19:24

youknowone reviewed Nov 9, 2021

View reviewed changes

add comments

ceafd4c

youknowone approved these changes Nov 9, 2021

View reviewed changes

qingshi163 merged commit 742ea0c into RustPython:main Nov 13, 2021

This was referenced Nov 28, 2021

Implement protocols (Abstract obejcts layer) #3244

Open

SaturatedSliceIter::next has overflow bug #3468

Closed

qingshi163 deleted the fix-buffer branch July 14, 2023 15:00

	pub struct SaturatedSliceIterator {
	pub struct SaturatedSliceIter {

	pub unsafe fn contiguous(&self) -> BorrowedValue<[u8]> {
	pub unsafe fn contiguous_unchecked(&self) -> BorrowedValue<[u8]> {

	pub fn collect(&self, buf: &mut Vec<u8>) {
	pub fn append_to(&self, buf: &mut Vec<u8>) {

		@@ -123,17 +147,293 @@ impl TryFromBorrowedObject for PyBuffer {
		// but it is not supported by Rust

Conversation

qingshi163 commented Oct 19, 2021

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!

youknowone commented Oct 25, 2021

Uh oh!

qingshi163 commented Oct 25, 2021

Uh oh!

youknowone commented Oct 26, 2021

Uh oh!

qingshi163 commented Oct 28, 2021

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!

qingshi163 commented Nov 7, 2021

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!

qingshi163 commented Nov 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants