From 06c95432c6a5b280e13c81a9802c6f7561a8fcf6 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 16 Jan 2024 22:53:38 +0000 Subject: [PATCH 1/2] set & frozenset bound constructors --- src/conversions/hashbrown.rs | 5 +-- src/conversions/std/set.rs | 10 +++--- src/types/frozenset.rs | 60 +++++++++++++++++++++++++++--------- src/types/set.rs | 55 ++++++++++++++++++++++++++------- 4 files changed, 97 insertions(+), 33 deletions(-) diff --git a/src/conversions/hashbrown.rs b/src/conversions/hashbrown.rs index 62a7e87b..a315a274 100644 --- a/src/conversions/hashbrown.rs +++ b/src/conversions/hashbrown.rs @@ -109,6 +109,7 @@ where #[cfg(test)] mod tests { use super::*; + use crate::types::any::PyAnyMethods; #[test] fn test_hashbrown_hashmap_to_python() { @@ -178,11 +179,11 @@ mod tests { #[test] fn test_extract_hashbrown_hashset() { Python::with_gil(|py| { - let set = PySet::new(py, &[1, 2, 3, 4, 5]).unwrap(); + let set = PySet::new_bound(py, &[1, 2, 3, 4, 5]).unwrap(); let hash_set: hashbrown::HashSet = set.extract().unwrap(); assert_eq!(hash_set, [1, 2, 3, 4, 5].iter().copied().collect()); - let set = PyFrozenSet::new(py, &[1, 2, 3, 4, 5]).unwrap(); + let set = PyFrozenSet::new_bound(py, &[1, 2, 3, 4, 5]).unwrap(); let hash_set: hashbrown::HashSet = set.extract().unwrap(); assert_eq!(hash_set, [1, 2, 3, 4, 5].iter().copied().collect()); }); diff --git a/src/conversions/std/set.rs b/src/conversions/std/set.rs index 4221c1ff..020b2505 100644 --- a/src/conversions/std/set.rs +++ b/src/conversions/std/set.rs @@ -113,18 +113,18 @@ where #[cfg(test)] mod tests { - use super::{PyFrozenSet, PySet}; + use crate::types::{any::PyAnyMethods, PyFrozenSet, PySet}; use crate::{IntoPy, PyObject, Python, ToPyObject}; use std::collections::{BTreeSet, HashSet}; #[test] fn test_extract_hashset() { Python::with_gil(|py| { - let set = PySet::new(py, &[1, 2, 3, 4, 5]).unwrap(); + let set = PySet::new_bound(py, &[1, 2, 3, 4, 5]).unwrap(); let hash_set: HashSet = set.extract().unwrap(); assert_eq!(hash_set, [1, 2, 3, 4, 5].iter().copied().collect()); - let set = PyFrozenSet::new(py, &[1, 2, 3, 4, 5]).unwrap(); + let set = PyFrozenSet::new_bound(py, &[1, 2, 3, 4, 5]).unwrap(); let hash_set: HashSet = set.extract().unwrap(); assert_eq!(hash_set, [1, 2, 3, 4, 5].iter().copied().collect()); }); @@ -133,11 +133,11 @@ mod tests { #[test] fn test_extract_btreeset() { Python::with_gil(|py| { - let set = PySet::new(py, &[1, 2, 3, 4, 5]).unwrap(); + let set = PySet::new_bound(py, &[1, 2, 3, 4, 5]).unwrap(); let hash_set: BTreeSet = set.extract().unwrap(); assert_eq!(hash_set, [1, 2, 3, 4, 5].iter().copied().collect()); - let set = PyFrozenSet::new(py, &[1, 2, 3, 4, 5]).unwrap(); + let set = PyFrozenSet::new_bound(py, &[1, 2, 3, 4, 5]).unwrap(); let hash_set: BTreeSet = set.extract().unwrap(); assert_eq!(hash_set, [1, 2, 3, 4, 5].iter().copied().collect()); }); diff --git a/src/types/frozenset.rs b/src/types/frozenset.rs index ff0be7b5..fa56a9c6 100644 --- a/src/types/frozenset.rs +++ b/src/types/frozenset.rs @@ -1,10 +1,12 @@ -#[cfg(not(Py_LIMITED_API))] -use crate::ffi_ptr_ext::FfiPtrExt; #[cfg(Py_LIMITED_API)] use crate::types::PyIterator; use crate::{ err::{self, PyErr, PyResult}, - ffi, Bound, Py, PyAny, PyNativeType, PyObject, Python, ToPyObject, + ffi, + ffi_ptr_ext::FfiPtrExt, + py_result_ext::PyResultExt, + types::any::PyAnyMethods, + Bound, PyAny, PyNativeType, PyObject, Python, ToPyObject, }; use std::ptr; @@ -63,20 +65,45 @@ pyobject_native_type_core!( ); impl PyFrozenSet { - /// Creates a new frozenset. - /// - /// May panic when running out of memory. + /// Deprecated form of [`PyFrozenSet::new_bound`]. #[inline] + #[cfg_attr( + not(feature = "gil-refs"), + deprecated( + since = "0.21.0", + note = "`PyFrozenSet::new` will be replaced by `PyFrozenSet::new_bound` in a future PyO3 version" + ) + )] pub fn new<'a, 'p, T: ToPyObject + 'a>( py: Python<'p>, elements: impl IntoIterator, ) -> PyResult<&'p PyFrozenSet> { - new_from_iter(py, elements).map(|set| set.into_ref(py)) + Self::new_bound(py, elements).map(Bound::into_gil_ref) + } + + /// Creates a new frozenset. + /// + /// May panic when running out of memory. + #[inline] + pub fn new_bound<'a, 'p, T: ToPyObject + 'a>( + py: Python<'p>, + elements: impl IntoIterator, + ) -> PyResult> { + new_from_iter(py, elements) + } + + /// Deprecated form of [`PyFrozenSet::empty_bound`]. + pub fn empty(py: Python<'_>) -> PyResult<&'_ PyFrozenSet> { + Self::empty_bound(py).map(Bound::into_gil_ref) } /// Creates a new empty frozen set - pub fn empty(py: Python<'_>) -> PyResult<&PyFrozenSet> { - unsafe { py.from_owned_ptr_or_err(ffi::PyFrozenSet_New(ptr::null_mut())) } + pub fn empty_bound(py: Python<'_>) -> PyResult> { + unsafe { + ffi::PyFrozenSet_New(ptr::null_mut()) + .assume_owned_or_err(py) + .downcast_into_unchecked() + } } /// Return the number of items in the set. @@ -285,14 +312,16 @@ pub use impl_::*; pub(crate) fn new_from_iter( py: Python<'_>, elements: impl IntoIterator, -) -> PyResult> { - fn inner( - py: Python<'_>, +) -> PyResult> { + fn inner<'py>( + py: Python<'py>, elements: &mut dyn Iterator, - ) -> PyResult> { - let set: Py = unsafe { + ) -> PyResult> { + let set = unsafe { // We create the `Py` pointer because its Drop cleans up the set if user code panics. - Py::from_owned_ptr_or_err(py, ffi::PyFrozenSet_New(std::ptr::null_mut()))? + ffi::PyFrozenSet_New(std::ptr::null_mut()) + .assume_owned_or_err(py)? + .downcast_into_unchecked() }; let ptr = set.as_ptr(); @@ -308,6 +337,7 @@ pub(crate) fn new_from_iter( } #[cfg(test)] +#[cfg_attr(not(feature = "gil-refs"), allow(deprecated))] mod tests { use super::*; diff --git a/src/types/set.rs b/src/types/set.rs index d59a9e3f..3204e71f 100644 --- a/src/types/set.rs +++ b/src/types/set.rs @@ -4,7 +4,9 @@ use crate::{ err::{self, PyErr, PyResult}, ffi_ptr_ext::FfiPtrExt, instance::Bound, - Py, PyNativeType, + py_result_ext::PyResultExt, + types::any::PyAnyMethods, + PyNativeType, }; use crate::{ffi, PyAny, PyObject, Python, ToPyObject}; use std::ptr; @@ -29,20 +31,45 @@ pyobject_native_type_core!( ); impl PySet { - /// Creates a new set with elements from the given slice. - /// - /// Returns an error if some element is not hashable. + /// Deprecated form of [`PySet::new_bound`]. + #[cfg_attr( + not(feature = "gil-refs"), + deprecated( + since = "0.21.0", + note = "`PySet::new` will be replaced by `PySet::new_bound` in a future PyO3 version" + ) + )] #[inline] pub fn new<'a, 'p, T: ToPyObject + 'a>( py: Python<'p>, elements: impl IntoIterator, ) -> PyResult<&'p PySet> { - new_from_iter(py, elements).map(|set| set.into_ref(py)) + Self::new_bound(py, elements).map(Bound::into_gil_ref) + } + + /// Creates a new set with elements from the given slice. + /// + /// Returns an error if some element is not hashable. + #[inline] + pub fn new_bound<'a, 'p, T: ToPyObject + 'a>( + py: Python<'p>, + elements: impl IntoIterator, + ) -> PyResult> { + new_from_iter(py, elements) + } + + /// Deprecated form of [`PySet::empty_bound`]. + pub fn empty(py: Python<'_>) -> PyResult<&'_ PySet> { + Self::empty_bound(py).map(Bound::into_gil_ref) } /// Creates a new empty set. - pub fn empty(py: Python<'_>) -> PyResult<&PySet> { - unsafe { py.from_owned_ptr_or_err(ffi::PySet_New(ptr::null_mut())) } + pub fn empty_bound(py: Python<'_>) -> PyResult> { + unsafe { + ffi::PySet_New(ptr::null_mut()) + .assume_owned_or_err(py) + .downcast_into_unchecked() + } } /// Removes all elements from the set. @@ -379,11 +406,16 @@ pub use impl_::*; pub(crate) fn new_from_iter( py: Python<'_>, elements: impl IntoIterator, -) -> PyResult> { - fn inner(py: Python<'_>, elements: &mut dyn Iterator) -> PyResult> { - let set: Py = unsafe { +) -> PyResult> { + fn inner<'py>( + py: Python<'py>, + elements: &mut dyn Iterator, + ) -> PyResult> { + let set = unsafe { // We create the `Py` pointer because its Drop cleans up the set if user code panics. - Py::from_owned_ptr_or_err(py, ffi::PySet_New(std::ptr::null_mut()))? + ffi::PySet_New(std::ptr::null_mut()) + .assume_owned_or_err(py)? + .downcast_into_unchecked() }; let ptr = set.as_ptr(); @@ -399,6 +431,7 @@ pub(crate) fn new_from_iter( } #[cfg(test)] +#[cfg_attr(not(feature = "gil-refs"), allow(deprecated))] mod tests { use super::PySet; use crate::{Python, ToPyObject}; From 4e24e680e874832c61248b284e2b5de2a4a2c7c5 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 17 Jan 2024 10:49:21 +0000 Subject: [PATCH 2/2] update set benchmarks --- pyo3-benches/benches/bench_set.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/pyo3-benches/benches/bench_set.rs b/pyo3-benches/benches/bench_set.rs index 0753a2f9..49243a63 100644 --- a/pyo3-benches/benches/bench_set.rs +++ b/pyo3-benches/benches/bench_set.rs @@ -10,21 +10,17 @@ fn set_new(b: &mut Bencher<'_>) { // Create Python objects up-front, so that the benchmark doesn't need to include // the cost of allocating LEN Python integers let elements: Vec = (0..LEN).map(|i| i.into_py(py)).collect(); - b.iter(|| { - let pool = unsafe { py.new_pool() }; - PySet::new(py, &elements).unwrap(); - drop(pool); - }); + b.iter_with_large_drop(|| PySet::new_bound(py, &elements).unwrap()); }); } fn iter_set(b: &mut Bencher<'_>) { Python::with_gil(|py| { const LEN: usize = 100_000; - let set = PySet::new(py, &(0..LEN).collect::>()).unwrap(); + let set = PySet::new_bound(py, &(0..LEN).collect::>()).unwrap(); let mut sum = 0; b.iter(|| { - for x in set { + for x in set.iter() { let i: u64 = x.extract().unwrap(); sum += i; } @@ -35,16 +31,16 @@ fn iter_set(b: &mut Bencher<'_>) { fn extract_hashset(b: &mut Bencher<'_>) { Python::with_gil(|py| { const LEN: usize = 100_000; - let set = PySet::new(py, &(0..LEN).collect::>()).unwrap(); - b.iter(|| HashSet::::extract(set)); + let set = PySet::new_bound(py, &(0..LEN).collect::>()).unwrap(); + b.iter_with_large_drop(|| HashSet::::extract(set.as_gil_ref())); }); } fn extract_btreeset(b: &mut Bencher<'_>) { Python::with_gil(|py| { const LEN: usize = 100_000; - let set = PySet::new(py, &(0..LEN).collect::>()).unwrap(); - b.iter(|| BTreeSet::::extract(set)); + let set = PySet::new_bound(py, &(0..LEN).collect::>()).unwrap(); + b.iter_with_large_drop(|| BTreeSet::::extract(set.as_gil_ref())); }); } @@ -52,8 +48,8 @@ fn extract_btreeset(b: &mut Bencher<'_>) { fn extract_hashbrown_set(b: &mut Bencher<'_>) { Python::with_gil(|py| { const LEN: usize = 100_000; - let set = PySet::new(py, &(0..LEN).collect::>()).unwrap(); - b.iter(|| hashbrown::HashSet::::extract(set)); + let set = PySet::new_bound(py, &(0..LEN).collect::>()).unwrap(); + b.iter_with_large_drop(|| hashbrown::HashSet::::extract(set.as_gil_ref())); }); }