From 6d442e59692273c19d7a8883ab220dbde9d36d00 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Wed, 14 Nov 2018 15:39:07 +0900 Subject: [PATCH 1/3] Use NonNull in release pool --- src/object.rs | 6 +++ src/python.rs | 102 +++++++++++++++++++++++------------------------ src/pythonrun.rs | 41 +++++++++---------- 3 files changed, 78 insertions(+), 71 deletions(-) diff --git a/src/object.rs b/src/object.rs index 87b0e01c684..d0cfd9f5d8a 100644 --- a/src/object.rs +++ b/src/object.rs @@ -33,6 +33,12 @@ impl PyObject { PyObject(ptr) } + pub(crate) unsafe fn into_nonnull(self) -> NonNull { + let res = self.0; + std::mem::forget(self); // Avoid Drop + res + } + /// Creates a `PyObject` instance for the given FFI pointer. /// This moves ownership over the pointer into the `PyObject`. /// Undefined behavior if the pointer is NULL or invalid. diff --git a/src/python.rs b/src/python.rs index 1f96b88b61d..76b666daee1 100644 --- a/src/python.rs +++ b/src/python.rs @@ -14,6 +14,7 @@ use crate::types::{PyDict, PyModule, PyObjectRef, PyType}; use std; use std::ffi::CString; use std::marker::PhantomData; +use std::ptr::NonNull; use std::os::raw::c_int; /// Marker type that indicates that the GIL is currently held. @@ -308,7 +309,7 @@ impl<'p> Python<'p> { { let p; unsafe { - p = pythonrun::register_owned(self, obj.into_ptr()); + p = pythonrun::register_owned(self, obj.into_nonnull()); } ::try_from(p) } @@ -318,17 +319,15 @@ impl<'p> Python<'p> { where T: PyTypeInfo, { - let p = pythonrun::register_owned(self, obj.into_ptr()); + let p = pythonrun::register_owned(self, obj.into_nonnull()); self.unchecked_downcast(p) } /// Register `ffi::PyObject` pointer in release pool - pub unsafe fn from_borrowed_ptr_to_obj(self, ptr: *mut ffi::PyObject) -> &'p PyObjectRef { - if ptr.is_null() { - crate::err::panic_after_error(); - } else { - pythonrun::register_borrowed(self, ptr) + match NonNull::new(ptr) { + Some(p) => pythonrun::register_borrowed(self, p), + None => crate::err::panic_after_error(), } } @@ -339,11 +338,12 @@ impl<'p> Python<'p> { where T: PyTypeInfo, { - if ptr.is_null() { - crate::err::panic_after_error(); - } else { - let p = pythonrun::register_owned(self, ptr); - self.unchecked_downcast(p) + match NonNull::new(ptr) { + Some(p) => { + let p = pythonrun::register_owned(self, p); + self.unchecked_downcast(p) + } + None => crate::err::panic_after_error(), } } @@ -353,56 +353,58 @@ impl<'p> Python<'p> { where T: PyTypeInfo, { - if ptr.is_null() { - crate::err::panic_after_error(); - } else { - let p = pythonrun::register_owned(self, ptr); - self.unchecked_mut_downcast(p) + match NonNull::new(ptr) { + Some(p) => { + let p = pythonrun::register_owned(self, p); + self.unchecked_mut_downcast(p) + } + None => crate::err::panic_after_error(), } } /// Register owned `ffi::PyObject` pointer in release pool. /// Returns `Err(PyErr)` if the pointer is `null`. /// do unchecked downcast to specific type. - pub unsafe fn from_owned_ptr_or_err(self, ptr: *mut ffi::PyObject) -> PyResult<&'p T> where T: PyTypeInfo, { - if ptr.is_null() { - Err(PyErr::fetch(self)) - } else { - let p = pythonrun::register_owned(self, ptr); - Ok(self.unchecked_downcast(p)) + match NonNull::new(ptr) { + Some(p) => { + let p = pythonrun::register_owned(self, p); + Ok(self.unchecked_downcast(p)) + } + None => Err(PyErr::fetch(self)), } } /// Register owned `ffi::PyObject` pointer in release pool. /// Returns `None` if the pointer is `null`. /// do unchecked downcast to specific type. - pub unsafe fn from_owned_ptr_or_opt(self, ptr: *mut ffi::PyObject) -> Option<&'p T> where T: PyTypeInfo, { - if ptr.is_null() { - None - } else { - let p = pythonrun::register_owned(self, ptr); - Some(self.unchecked_downcast(p)) - } + NonNull::new(ptr).map(|p| { + let p = pythonrun::register_owned(self, p); + self.unchecked_downcast(p) + }) } /// Register borrowed `ffi::PyObject` pointer in release pool. /// Panics if the pointer is `null`. /// do unchecked downcast to specific type. - pub unsafe fn from_borrowed_ptr(self, ptr: *mut ffi::PyObject) -> &'p T where T: PyTypeInfo, { - let p = pythonrun::register_borrowed(self, ptr); - self.unchecked_downcast(p) + match NonNull::new(ptr) { + Some(p) => { + let p = pythonrun::register_borrowed(self, p); + self.unchecked_downcast(p) + } + None => crate::err::panic_after_error(), + } } /// Register borrowed `ffi::PyObject` pointer in release pool. @@ -412,44 +414,42 @@ impl<'p> Python<'p> { where T: PyTypeInfo, { - if ptr.is_null() { - crate::err::panic_after_error(); - } else { - let p = pythonrun::register_borrowed(self, ptr); - self.unchecked_mut_downcast(p) + match NonNull::new(ptr) { + Some(p) => { + let p = pythonrun::register_borrowed(self, p); + self.unchecked_mut_downcast(p) + } + None => crate::err::panic_after_error(), } } /// Register borrowed `ffi::PyObject` pointer in release pool. /// Returns `Err(PyErr)` if the pointer is `null`. /// do unchecked downcast to specific type. - pub unsafe fn from_borrowed_ptr_or_err(self, ptr: *mut ffi::PyObject) -> PyResult<&'p T> where T: PyTypeInfo, { - if ptr.is_null() { - Err(PyErr::fetch(self)) - } else { - let p = pythonrun::register_borrowed(self, ptr); - Ok(self.unchecked_downcast(p)) + match NonNull::new(ptr) { + Some(p) => { + let p = pythonrun::register_borrowed(self, p); + Ok(self.unchecked_downcast(p)) + } + None => Err(PyErr::fetch(self)), } } /// Register borrowed `ffi::PyObject` pointer in release pool. /// Returns `None` if the pointer is `null`. /// do unchecked downcast to specific `T`. - pub unsafe fn from_borrowed_ptr_or_opt(self, ptr: *mut ffi::PyObject) -> Option<&'p T> where T: PyTypeInfo, { - if ptr.is_null() { - None - } else { - let p = pythonrun::register_borrowed(self, ptr); - Some(self.unchecked_downcast(p)) - } + NonNull::new(ptr).map(|p| { + let p = pythonrun::register_borrowed(self, p); + self.unchecked_downcast(p) + }) } #[doc(hidden)] diff --git a/src/pythonrun.rs b/src/pythonrun.rs index 1005e8141c9..af19fef144e 100644 --- a/src/pythonrun.rs +++ b/src/pythonrun.rs @@ -108,11 +108,11 @@ impl Drop for GILGuard { /// Release pool struct ReleasePool { - owned: Vec<*mut ffi::PyObject>, - borrowed: Vec<*mut ffi::PyObject>, - pointers: *mut Vec<*mut ffi::PyObject>, + owned: Vec>, + borrowed: Vec>, + pointers: *mut Vec>, obj: Vec>, - p: spin::Mutex<*mut Vec<*mut ffi::PyObject>>, + p: spin::Mutex<*mut Vec>>, } impl ReleasePool { @@ -131,7 +131,7 @@ impl ReleasePool { // vec of pointers let ptr = *v; - let vec: &'static mut Vec<*mut ffi::PyObject> = &mut *ptr; + let vec: &'static mut Vec<_> = &mut *ptr; if vec.is_empty() { return; } @@ -143,7 +143,7 @@ impl ReleasePool { // release py objects for ptr in vec.iter_mut() { - ffi::Py_DECREF(*ptr); + ffi::Py_DECREF(ptr.as_ptr()); } vec.set_len(0); } @@ -152,7 +152,7 @@ impl ReleasePool { let len = self.owned.len(); if owned < len { for ptr in &mut self.owned[owned..len] { - ffi::Py_DECREF(*ptr); + ffi::Py_DECREF(ptr.as_ptr()); } self.owned.set_len(owned); } @@ -235,20 +235,20 @@ pub unsafe fn register_pointer(obj: NonNull) { let pool: &'static mut ReleasePool = &mut *POOL; let mut v = pool.p.lock(); - let pool: &'static mut Vec<*mut ffi::PyObject> = &mut *(*v); - pool.push(obj.as_ptr()); + let pool: &'static mut Vec<_> = &mut *(*v); + pool.push(obj); } -pub unsafe fn register_owned(_py: Python, obj: *mut ffi::PyObject) -> &PyObjectRef { +pub unsafe fn register_owned(_py: Python, obj: NonNull) -> &PyObjectRef { let pool: &'static mut ReleasePool = &mut *POOL; pool.owned.push(obj); - &*(&pool.owned[pool.owned.len() - 1] as *const *mut ffi::PyObject as *const PyObjectRef) + &*(&pool.owned[pool.owned.len() - 1] as *const _ as *const PyObjectRef) } -pub unsafe fn register_borrowed(_py: Python, obj: *mut ffi::PyObject) -> &PyObjectRef { +pub unsafe fn register_borrowed(_py: Python, obj: NonNull) -> &PyObjectRef { let pool: &'static mut ReleasePool = &mut *POOL; pool.borrowed.push(obj); - &*(&pool.borrowed[pool.borrowed.len() - 1] as *const *mut ffi::PyObject as *const PyObjectRef) + &*(&pool.borrowed[pool.borrowed.len() - 1] as *const _ as *const PyObjectRef) } impl GILGuard { @@ -280,7 +280,7 @@ impl GILGuard { #[cfg(test)] mod test { - use super::{GILPool, ReleasePool, POOL}; + use super::{GILPool, ReleasePool, POOL, NonNull}; use crate::conversion::ToPyObject; use crate::object::PyObject; use crate::python::{Python, ToPyPointer}; @@ -311,7 +311,7 @@ mod test { empty = ffi::PyTuple_New(0); cnt = ffi::Py_REFCNT(empty) - 1; - let _ = pythonrun::register_owned(py, empty); + let _ = pythonrun::register_owned(py, NonNull::new_unchecked(empty)); assert_eq!(p.owned.len(), 1); } @@ -341,14 +341,15 @@ mod test { // empty tuple is singleton empty = ffi::PyTuple_New(0); cnt = ffi::Py_REFCNT(empty) - 1; - let _ = pythonrun::register_owned(py, empty); + + let _ = pythonrun::register_owned(py, NonNull::new_unchecked(empty)); assert_eq!(p.owned.len(), 1); { let _pool = GILPool::new(); let empty = ffi::PyTuple_New(0); - let _ = pythonrun::register_owned(py, empty); + let _ = pythonrun::register_owned(py, NonNull::new_unchecked(empty)); assert_eq!(p.owned.len(), 2); } assert_eq!(p.owned.len(), 1); @@ -376,7 +377,7 @@ mod test { assert_eq!(p.borrowed.len(), 0); cnt = ffi::Py_REFCNT(obj_ptr); - pythonrun::register_borrowed(py, obj_ptr); + pythonrun::register_borrowed(py, NonNull::new_unchecked(obj_ptr)); assert_eq!(p.borrowed.len(), 1); assert_eq!(ffi::Py_REFCNT(obj_ptr), cnt); @@ -405,7 +406,7 @@ mod test { assert_eq!(p.borrowed.len(), 0); cnt = ffi::Py_REFCNT(obj_ptr); - pythonrun::register_borrowed(py, obj_ptr); + pythonrun::register_borrowed(py, NonNull::new_unchecked(obj_ptr)); assert_eq!(p.borrowed.len(), 1); assert_eq!(ffi::Py_REFCNT(obj_ptr), cnt); @@ -413,7 +414,7 @@ mod test { { let _pool = GILPool::new(); assert_eq!(p.borrowed.len(), 1); - pythonrun::register_borrowed(py, obj_ptr); + pythonrun::register_borrowed(py, NonNull::new_unchecked(obj_ptr)); assert_eq!(p.borrowed.len(), 2); } From 6222232c2105f04c72de7c900f8db57231145c0d Mon Sep 17 00:00:00 2001 From: kngwyu Date: Wed, 14 Nov 2018 15:40:08 +0900 Subject: [PATCH 2/3] Add bench_dict --- benches/bench_dict.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 benches/bench_dict.rs diff --git a/benches/bench_dict.rs b/benches/bench_dict.rs new file mode 100644 index 00000000000..620550da2fe --- /dev/null +++ b/benches/bench_dict.rs @@ -0,0 +1,21 @@ +#![feature(test)] +extern crate pyo3; +extern crate test; +use test::Bencher; + +use pyo3::{prelude::*, types::IntoPyDict}; + +#[bench] +fn iter_dict(b: &mut Bencher) { + let gil = Python::acquire_gil(); + let py = gil.python(); + const LEN: usize = 1_000_000; + let dict = (0..LEN as u64).map(|i| (i, i * 2)).into_py_dict(py); + let mut sum = 0; + b.iter(|| { + for (k, _v) in dict.iter() { + let i: u64 = k.extract().unwrap(); + sum += i; + } + }); +} From a73bd06c1f8b64627a86c277e4c73917ec4408a7 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Thu, 15 Nov 2018 13:40:35 +0900 Subject: [PATCH 3/3] Replace NonNull::new_unchecked with new().unwrap() in test --- src/pythonrun.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/pythonrun.rs b/src/pythonrun.rs index af19fef144e..c421504c5a0 100644 --- a/src/pythonrun.rs +++ b/src/pythonrun.rs @@ -311,7 +311,7 @@ mod test { empty = ffi::PyTuple_New(0); cnt = ffi::Py_REFCNT(empty) - 1; - let _ = pythonrun::register_owned(py, NonNull::new_unchecked(empty)); + let _ = pythonrun::register_owned(py, NonNull::new(empty).unwrap()); assert_eq!(p.owned.len(), 1); } @@ -342,14 +342,14 @@ mod test { empty = ffi::PyTuple_New(0); cnt = ffi::Py_REFCNT(empty) - 1; - let _ = pythonrun::register_owned(py, NonNull::new_unchecked(empty)); + let _ = pythonrun::register_owned(py, NonNull::new(empty).unwrap()); assert_eq!(p.owned.len(), 1); { let _pool = GILPool::new(); let empty = ffi::PyTuple_New(0); - let _ = pythonrun::register_owned(py, NonNull::new_unchecked(empty)); + let _ = pythonrun::register_owned(py, NonNull::new(empty).unwrap()); assert_eq!(p.owned.len(), 2); } assert_eq!(p.owned.len(), 1); @@ -377,7 +377,7 @@ mod test { assert_eq!(p.borrowed.len(), 0); cnt = ffi::Py_REFCNT(obj_ptr); - pythonrun::register_borrowed(py, NonNull::new_unchecked(obj_ptr)); + pythonrun::register_borrowed(py, NonNull::new(obj_ptr).unwrap()); assert_eq!(p.borrowed.len(), 1); assert_eq!(ffi::Py_REFCNT(obj_ptr), cnt); @@ -406,7 +406,7 @@ mod test { assert_eq!(p.borrowed.len(), 0); cnt = ffi::Py_REFCNT(obj_ptr); - pythonrun::register_borrowed(py, NonNull::new_unchecked(obj_ptr)); + pythonrun::register_borrowed(py, NonNull::new(obj_ptr).unwrap()); assert_eq!(p.borrowed.len(), 1); assert_eq!(ffi::Py_REFCNT(obj_ptr), cnt); @@ -414,7 +414,7 @@ mod test { { let _pool = GILPool::new(); assert_eq!(p.borrowed.len(), 1); - pythonrun::register_borrowed(py, NonNull::new_unchecked(obj_ptr)); + pythonrun::register_borrowed(py, NonNull::new(obj_ptr).unwrap()); assert_eq!(p.borrowed.len(), 2); }