Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy type objects without once_cell #758

Merged
merged 4 commits into from
Feb 8, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ parking_lot = { version = "0.10", features = ["nightly"] }
paste = "0.1.6"
pyo3cls = { path = "pyo3cls", version = "=0.9.0-alpha.1" }
unindent = "0.1.4"
once_cell = "1.3.1"

[dev-dependencies]
assert_approx_eq = "1.1.0"
Expand Down
8 changes: 4 additions & 4 deletions guide/src/class.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ unsafe impl pyo3::PyTypeInfo for MyClass {
const FLAGS: usize = 0;

#[inline]
fn type_object() -> std::ptr::NonNull<pyo3::ffi::PyTypeObject> {
use pyo3::type_object::LazyTypeObject;
static TYPE_OBJECT: LazyTypeObject = LazyTypeObject::new();
TYPE_OBJECT.get_pyclass_type::<Self>()
fn type_object() -> &'static pyo3::ffi::PyTypeObject {
use pyo3::type_object::LazyStaticType;
static TYPE_OBJECT: LazyStaticType = LazyStaticType::new();
TYPE_OBJECT.get::<Self>()
}
}

Expand Down
8 changes: 4 additions & 4 deletions pyo3-derive-backend/src/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,10 +382,10 @@ fn impl_class(
const FLAGS: usize = #(#flags)|* | #extended;

#[inline]
fn type_object() -> std::ptr::NonNull<pyo3::ffi::PyTypeObject> {
use pyo3::type_object::LazyTypeObject;
static TYPE_OBJECT: LazyTypeObject = LazyTypeObject::new();
TYPE_OBJECT.get_pyclass_type::<Self>()
fn type_object() -> &'static pyo3::ffi::PyTypeObject {
use pyo3::type_object::LazyStaticType;
static TYPE_OBJECT: LazyStaticType = LazyStaticType::new();
TYPE_OBJECT.get::<Self>()
}
}

Expand Down
74 changes: 32 additions & 42 deletions src/exceptions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ use crate::types::{PyAny, PyTuple};
use crate::Python;
use crate::{AsPyPointer, ToPyObject};
use std::ffi::CStr;
use std::ops;
use std::os::raw::c_char;
use std::{self, ops};

/// The boilerplate to convert between a rust type and a python exception
#[macro_export]
Expand Down Expand Up @@ -90,31 +90,26 @@ macro_rules! import_exception_type_object {
($module: expr, $name: ident) => {
unsafe impl $crate::type_object::PyTypeObject for $name {
fn type_object() -> $crate::Py<$crate::types::PyType> {
use $crate::type_object::LazyTypeObject;
static TYPE_OBJECT: LazyTypeObject = LazyTypeObject::new();

let ptr = TYPE_OBJECT
.get_or_init(|| {
let gil = $crate::Python::acquire_gil();
let py = gil.python();

let imp = py
.import(stringify!($module))
.expect(concat!("Can not import module: ", stringify!($module)));
let cls = imp.get(stringify!($name)).expect(concat!(
"Can not load exception class: {}.{}",
stringify!($module),
".",
stringify!($name)
));

unsafe {
Ok(std::ptr::NonNull::new_unchecked(
$crate::IntoPyPointer::into_ptr(cls) as *mut _,
))
}
})
.unwrap();
use $crate::type_object::LazyHeapType;
static TYPE_OBJECT: LazyHeapType = LazyHeapType::new();

let ptr = TYPE_OBJECT.get_or_init(|py| {
let imp = py
.import(stringify!($module))
.expect(concat!("Can not import module: ", stringify!($module)));
let cls = imp.get(stringify!($name)).expect(concat!(
"Can not load exception class: {}.{}",
stringify!($module),
".",
stringify!($name)
));

unsafe {
std::ptr::NonNull::new_unchecked(
$crate::IntoPyPointer::into_ptr(cls) as *mut _
)
}
});

unsafe { $crate::Py::from_borrowed_ptr(ptr.as_ptr() as *mut $crate::ffi::PyObject) }
}
Expand Down Expand Up @@ -178,22 +173,17 @@ macro_rules! create_exception_type_object {
($module: ident, $name: ident, $base: ty) => {
unsafe impl $crate::type_object::PyTypeObject for $name {
fn type_object() -> $crate::Py<$crate::types::PyType> {
use $crate::type_object::LazyTypeObject;
static TYPE_OBJECT: LazyTypeObject = LazyTypeObject::new();

let ptr = TYPE_OBJECT
.get_or_init(|| {
let gil = $crate::Python::acquire_gil();
let py = gil.python();

Ok($crate::PyErr::new_type(
py,
concat!(stringify!($module), ".", stringify!($name)),
Some(py.get_type::<$base>()),
None,
))
})
.unwrap();
use $crate::type_object::LazyHeapType;
static TYPE_OBJECT: LazyHeapType = LazyHeapType::new();

let ptr = TYPE_OBJECT.get_or_init(|py| {
$crate::PyErr::new_type(
py,
concat!(stringify!($module), ".", stringify!($name)),
Some(py.get_type::<$base>()),
None,
)
});

unsafe { $crate::Py::from_borrowed_ptr(ptr.as_ptr() as *mut $crate::ffi::PyObject) }
}
Expand Down
4 changes: 2 additions & 2 deletions src/freelist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ where
{
unsafe fn alloc(_py: Python) -> *mut Self::ConcreteLayout {
if let Some(obj) = <Self as PyClassWithFreeList>::get_free_list().pop() {
ffi::PyObject_Init(obj, <Self as PyTypeInfo>::type_object().as_ptr() as *mut _);
ffi::PyObject_Init(obj, <Self as PyTypeInfo>::type_object() as *const _ as _);
obj as _
} else {
crate::pyclass::default_alloc::<Self>() as _
Expand All @@ -90,7 +90,7 @@ where
}

if let Some(obj) = <Self as PyClassWithFreeList>::get_free_list().insert(obj) {
match Self::type_object().as_ref().tp_free {
match Self::type_object().tp_free {
Some(free) => free(obj as *mut c_void),
None => tp_free_fallback(obj),
}
Expand Down
27 changes: 11 additions & 16 deletions src/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,17 @@ use std::ptr::{self, NonNull};

#[inline]
pub(crate) unsafe fn default_alloc<T: PyTypeInfo>() -> *mut ffi::PyObject {
let tp_ptr = T::type_object().as_ptr();
let type_obj = T::type_object();
if T::FLAGS & type_flags::EXTENDED != 0
&& <T::BaseType as PyTypeInfo>::ConcreteLayout::IS_NATIVE_TYPE
{
let base_tp = <T::BaseType as PyTypeInfo>::type_object();
if let Some(base_new) = base_tp.as_ref().tp_new {
return base_new(tp_ptr, ptr::null_mut(), ptr::null_mut());
if let Some(base_new) = base_tp.tp_new {
return base_new(type_obj as *const _ as _, ptr::null_mut(), ptr::null_mut());
}
}
let alloc = (*tp_ptr).tp_alloc.unwrap_or(ffi::PyType_GenericAlloc);
alloc(tp_ptr, 0)
let alloc = type_obj.tp_alloc.unwrap_or(ffi::PyType_GenericAlloc);
alloc(type_obj as *const _ as _, 0)
}

/// This trait enables custom alloc/dealloc implementations for `T: PyClass`.
Expand All @@ -47,7 +47,7 @@ pub trait PyClassAlloc: PyTypeInfo + Sized {
return;
}

match Self::type_object().as_ref().tp_free {
match Self::type_object().tp_free {
Some(free) => free(obj as *mut c_void),
None => tp_free_fallback(obj),
}
Expand Down Expand Up @@ -253,19 +253,14 @@ where
}

#[cfg(not(Py_LIMITED_API))]
pub(crate) fn create_type_object<T>(
pub(crate) fn initialize_type_object<T>(
py: Python,
module_name: Option<&str>,
) -> PyResult<Box<ffi::PyTypeObject>>
type_object: &mut ffi::PyTypeObject,
) -> PyResult<()>
where
T: PyClass,
{
// Box (or some other heap allocation) is needed because PyType_Ready expects the type object
// to have a permanent memory address.
let mut boxed = Box::new(ffi::PyTypeObject_INIT);
let mut type_object = boxed.as_mut();
let base_type_object = <T::BaseType as PyTypeInfo>::type_object().as_ptr();

// PyPy will segfault if passed only a nul terminator as `tp_doc`.
// ptr::null() is OK though.
if T::DESCRIPTION == "\0" {
Expand All @@ -274,7 +269,7 @@ where
type_object.tp_doc = T::DESCRIPTION.as_ptr() as *const _;
};

type_object.tp_base = base_type_object;
type_object.tp_base = <T::BaseType as PyTypeInfo>::type_object() as *const _ as _;

let name = match module_name {
Some(module_name) => format!("{}.{}", module_name, T::NAME),
Expand Down Expand Up @@ -371,7 +366,7 @@ where
// register type object
unsafe {
if ffi::PyType_Ready(type_object) == 0 {
Ok(boxed)
Ok(())
} else {
PyErr::fetch(py).into()
}
Expand Down
92 changes: 55 additions & 37 deletions src/type_object.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
// Copyright (c) 2017-present PyO3 Project and Contributors
//! Python type object information

use crate::err::PyResult;
use crate::instance::Py;
use crate::pyclass::{create_type_object, PyClass};
use crate::pyclass::{initialize_type_object, PyClass};
use crate::pyclass_init::PyObjectInit;
use crate::types::{PyAny, PyType};
use crate::{ffi, AsPyPointer, Python};
use once_cell::sync::OnceCell;
use std::cell::{Cell, UnsafeCell};
use std::ptr::NonNull;

/// `T: PyObjectLayout<U>` represents that `T` is a concrete representaion of `U` in Python heap.
Expand Down Expand Up @@ -92,19 +91,19 @@ pub unsafe trait PyTypeInfo: Sized {
/// Initializer for layout
type Initializer: PyObjectInit<Self>;

/// PyTypeObject instance for this type, guaranteed to be global and initialized.
fn type_object() -> NonNull<ffi::PyTypeObject>;
/// PyTypeObject instance for this type.
fn type_object() -> &'static ffi::PyTypeObject;

/// Check if `*mut ffi::PyObject` is instance of this type
fn is_instance(object: &PyAny) -> bool {
unsafe {
ffi::PyObject_TypeCheck(object.as_ptr(), Self::type_object().as_ptr() as *mut _) != 0
ffi::PyObject_TypeCheck(object.as_ptr(), Self::type_object() as *const _ as _) != 0
}
}

/// Check if `*mut ffi::PyObject` is exact instance of this type
fn is_exact_instance(object: &PyAny) -> bool {
unsafe { (*object.as_ptr()).ob_type == Self::type_object().as_ptr() as *mut _ }
unsafe { (*object.as_ptr()).ob_type == Self::type_object() as *const _ as _ }
}
}

Expand All @@ -124,49 +123,68 @@ where
T: PyTypeInfo,
{
fn type_object() -> Py<PyType> {
unsafe { Py::from_borrowed_ptr(<Self as PyTypeInfo>::type_object().as_ptr() as *mut _) }
unsafe { Py::from_borrowed_ptr(<Self as PyTypeInfo>::type_object() as *const _ as _) }
}
}

/// Type used to store static type objects
/// Lazy type object for Exceptions
#[doc(hidden)]
pub struct LazyTypeObject {
cell: OnceCell<NonNull<ffi::PyTypeObject>>,
}
pub struct LazyHeapType(UnsafeCell<Option<NonNull<ffi::PyTypeObject>>>);

impl LazyTypeObject {
impl LazyHeapType {
pub const fn new() -> Self {
Self {
cell: OnceCell::new(),
}
Self(UnsafeCell::new(None))
}

pub fn get_or_init<F>(&self, constructor: F) -> PyResult<NonNull<ffi::PyTypeObject>>
pub fn get_or_init<F>(&self, constructor: F) -> NonNull<ffi::PyTypeObject>
where
F: Fn() -> PyResult<NonNull<ffi::PyTypeObject>>,
F: Fn(Python) -> NonNull<ffi::PyTypeObject>,
{
Ok(*self.cell.get_or_try_init(constructor)?)
if let Some(value) = unsafe { &*self.0.get() } {
return value.clone();
}
// We have to get the GIL before setting the value to the global!!!
let gil = Python::acquire_gil();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unlikely to happen in practice, but there's nothing to prevent two or more threads reaching this point at the same time, which would then lead to value being initialized multiple times. Whether this is a problem, I'm not sure.

We could avoid this by checking again that value is still None after the GIL lock is acquired, and exiting early if some other thread initialized value while this thread was waiting for the GIL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.
I changed this to use AtomicBool

unsafe {
*self.0.get() = Some(constructor(gil.python()));
(*self.0.get()).unwrap()
}
}
}

pub fn get_pyclass_type<T: PyClass>(&self) -> NonNull<ffi::PyTypeObject> {
self.get_or_init(|| {
// automatically initialize the class on-demand
let gil = Python::acquire_gil();
let py = gil.python();
let boxed = create_type_object::<T>(py, T::MODULE)?;
Ok(unsafe { NonNull::new_unchecked(Box::into_raw(boxed)) })
})
.unwrap_or_else(|e| {
// This is necessary for making static `LazyHeapType`s
//
// Type objects are shared between threads by the Python interpreter anyway, so it is no worse
// to allow sharing on the Rust side too.
unsafe impl Sync for LazyHeapType {}

/// Lazy type object for PyClass
#[doc(hidden)]
pub struct LazyStaticType {
value: UnsafeCell<ffi::PyTypeObject>,
initialized: Cell<bool>,
}

impl LazyStaticType {
pub const fn new() -> Self {
LazyStaticType {
value: UnsafeCell::new(ffi::PyTypeObject_INIT),
initialized: Cell::new(false),
}
}
pub fn get<T: PyClass>(&self) -> &ffi::PyTypeObject {
if !self.initialized.get() {
let gil = Python::acquire_gil();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment applies as for LazyHeapType.

let py = gil.python();
e.print(py);
panic!("An error occurred while initializing class {}", T::NAME)
})
initialize_type_object::<T>(py, T::MODULE, unsafe { &mut *self.value.get() })
.unwrap_or_else(|e| {
e.print(py);
panic!("An error occurred while initializing class {}", T::NAME)
});
self.initialized.set(true);
}
unsafe { &*self.value.get() }
}
}

// This is necessary for making static `LazyTypeObject`s
//
// Type objects are shared between threads by the Python interpreter anyway, so it is no worse
// to allow sharing on the Rust side too.
unsafe impl Sync for LazyTypeObject {}
// This is necessary for making static `LazyStaticType`s
unsafe impl Sync for LazyStaticType {}
4 changes: 2 additions & 2 deletions src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ macro_rules! pyobject_native_type_convert(
const MODULE: Option<&'static str> = $module;

#[inline]
fn type_object() -> std::ptr::NonNull<$crate::ffi::PyTypeObject> {
unsafe { std::ptr::NonNull::new_unchecked(&mut $typeobject as *mut _) }
fn type_object() -> &'static $crate::ffi::PyTypeObject {
unsafe{ &$typeobject }
}

#[allow(unused_unsafe)]
Expand Down