RFC: Extend SliceBox and unify Array creation from owned data #231

adamreichold · 2021-12-30T08:39:09Z

@kngwyu This is a follow-up to #230 and #233 and strictly optional, but I think getting rid of the pointer manipulations in Array: IntoPyArray is nice and unifying the different code paths should reduce the maintenance burden.

adamreichold · 2021-12-30T10:41:46Z

The CI failure has #232 as its same root cause, however I am unsure how to fix this and still be able to store e.g. Array<A, D> within Owner which would make its size type-dependent. In this case, yielding separate PyTypeObjects for each T would certainly be preferable.

@davidhewitt Is it possible to make general statements on how often PyTypeInfo::type_object_raw is called, i.e. what the impact of having a type-ID-keyed hash table in there? Is there a way to construct a PyTypeObject without going through LazyStaticType?

davidhewitt · 2022-01-02T23:55:52Z

Sorry for the slow reply. My laptop keyboard is having issues so I'm not keeping up with all discussions at the moment.

type_object_raw may be called quite freuently, e.g. every time this class is created and also every time a Python type needs to be type checked.

kngwyu · 2022-01-04T11:16:41Z

Thank you! (and sorry for my late reply... we have long new-year holidays in Japan).
The implementation looks nice, but what is the motivation to support Vec<T> directly? Efficiency?

adamreichold · 2022-01-04T11:41:48Z

The implementation looks nice, but what is the motivation to support Vec directly? Efficiency?

Efficiency and simplicity: into_boxed_slice can re-allocate to shed excess capacity. Also the re-computation of the pointer to the first element is not necessary any more.

adamreichold · 2022-01-04T11:43:09Z

(By the way, I think it would only make sense to review this PR after #230 and #233 are done with it as this might change significantly based on the feedback there. Especially #233 as it seems required to avoid memory unsafety.)

kngwyu

Thanks, I left some comments,

kngwyu · 2022-01-06T08:31:09Z

src/owner.rs

+pub(crate) struct Owner {
+    ptr: *mut u8,
+    len: usize,
+    cap: usize,
+    drop: unsafe fn(*mut u8, usize, usize),
+}


Isn't Owner a too broad name? I prefer to PySliceContainer, SliceContainer, SliceOwner and so on.

Went back to the "container" naming, i.e. PySliceContainer for the type and container: C for the bindings.

kngwyu · 2022-01-06T08:32:21Z

src/owner.rs

+        let cap = 0;
+        let drop = drop_boxed_slice::<T>;
+
+        mem::forget(data);


I prefer to Box::into_raw for readability. We use mem::forget for various usages, but into_raw has only one purpose.

Me too. :-) The problem is that Box::into_raw will yield *mut [T], i.e. a fat pointer, for the boxed slice which does not apply to the Vec<_> case. Hence I manually deconstructed the boxed slice to be able to handle both types in a more or less uniform way.

Ahh OK I got it.

https://doc.rust-lang.org/std/primitive.pointer.html#method.as_mut_ptr this would work but it's still unstable too 😅

Or we can use std::slice::from_raw_parts for Vec side, but either approach is not so good, so either is fine for me.

Yes, I think the current approach is a reasonable compromise as it handles both types in the same way. I did add another FIXME comment w.r.t. Box::into_raw though.

kngwyu · 2022-01-06T08:34:03Z

src/owner.rs

+        Self {
+            ptr,
+            len,
+            cap,
+            drop,
+        }


Is there any reason to avoid

Self { ptr: data.as_ptr() as *mut u8, len: data.len(), cap: 0, drop: drop_boxed_slice::<T>, }

?
Readability?

The mem::forget call consumes data which is why I had to put these into separate bindings.

src/owner.rs

By providing type erasure for both Box<[T]> and Vec<T> we can avoid having to transform Vec<T> and Array<A, D> into boxed slices which can potentially re-allocate.

Both create an array from existing data, they just differ w.r.t. how the how the owner on the Python heap is handled.

kngwyu · 2022-01-06T09:49:31Z

src/slice_container.rs

+
+/// Utility type to safely store Box<[_]> or Vec<_> on the Python heap
+pub(crate) struct PySliceContainer {
+    ptr: *mut u8,


BTW, numpy does not allow mutating the array that has a parent array, so this can be *const in theory. So, let's try it if you are also curious if it works 😄

I think owning raw pointers are *mut by convention.

adamreichold · 2022-01-06T10:18:06Z

Just noticed that I did not add a changelog entry for PyArray::borrow_from_array. Technically it does not belong here, but I added the commit to this PR nevertheless as it is a follow-up and so somewhat related.

adamreichold changed the title ~~RFC: Simplify SliceBox and unify Array creation from owned data~~ RFC: Extend SliceBox and unify Array creation from owned data Dec 30, 2021

adamreichold mentioned this pull request Jan 3, 2022

Fix SliceBox-induced type confusion #233

Merged

kngwyu requested changes Jan 6, 2022

View reviewed changes

adamreichold added 3 commits January 6, 2022 10:20

Extend SliceBox into Owner which can hold onto both Box<[T]> and Vec<T>

f24bbdc

By providing type erasure for both Box<[T]> and Vec<T> we can avoid having to transform Vec<T> and Array<A, D> into boxed slices which can potentially re-allocate.

Unify implemenation of Array::from_raw_parts and ::borrow_from_array

dd675ba

Both create an array from existing data, they just differ w.r.t. how the how the owner on the Python heap is handled.

Use a more specific name for the Owner type, i.e. PySliceContainer.

7176bce

kngwyu approved these changes Jan 6, 2022

View reviewed changes

Fix missing changelog entry for PyArray::borrow_from_array.

3ebf4eb

adamreichold merged commit 0efa2cd into PyO3:main Jan 6, 2022

adamreichold deleted the simpler-slice-box branch January 6, 2022 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Extend SliceBox and unify Array creation from owned data #231

RFC: Extend SliceBox and unify Array creation from owned data #231

adamreichold commented Dec 30, 2021 •

edited

Loading

adamreichold commented Dec 30, 2021

davidhewitt commented Jan 2, 2022

kngwyu commented Jan 4, 2022

adamreichold commented Jan 4, 2022

adamreichold commented Jan 4, 2022

kngwyu left a comment

kngwyu Jan 6, 2022

adamreichold Jan 6, 2022

kngwyu Jan 6, 2022

adamreichold Jan 6, 2022

kngwyu Jan 6, 2022

kngwyu Jan 6, 2022

kngwyu Jan 6, 2022

adamreichold Jan 6, 2022

kngwyu Jan 6, 2022

adamreichold Jan 6, 2022

kngwyu Jan 6, 2022

adamreichold Jan 6, 2022

adamreichold commented Jan 6, 2022

RFC: Extend SliceBox and unify Array creation from owned data #231

RFC: Extend SliceBox and unify Array creation from owned data #231

Conversation

adamreichold commented Dec 30, 2021 • edited Loading

adamreichold commented Dec 30, 2021

davidhewitt commented Jan 2, 2022

kngwyu commented Jan 4, 2022

adamreichold commented Jan 4, 2022

adamreichold commented Jan 4, 2022

kngwyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamreichold commented Jan 6, 2022

adamreichold commented Dec 30, 2021 •

edited

Loading