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.

Sign up for GitHub

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

Jump to bottom

Add support for nogil Python #2885

Closed
wants to merge 4 commits into from
Closed

Add support for nogil Python #2885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9b9f22
Add support for nogil Python
colesbury Apr 21, 2022
b390838
Use sys.implementation.name instead of SOABI.
colesbury Jan 18, 2023
800c20d
Use assert!
colesbury Jan 18, 2023
17b4c15
Avoid unsafe borrowed references when using Py_NOGIL
colesbury Jan 18, 2023
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
17 changes: 15 additions & 2 deletions pyo3-build-config/src/impl_.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,10 @@ impl InterpreterConfig {
println!("cargo:rustc-cfg=Py_LIMITED_API");
}

if self.implementation == PythonImplementation::NoGIL {
println!("cargo:rustc-cfg=Py_NOGIL");
}

for flag in &self.build_flags.0 {
println!("cargo:rustc-cfg=py_sys_config=\"{}\"", flag);
}
Expand Down Expand Up @@ -220,7 +224,11 @@ FRAMEWORK = bool(get_config_var("PYTHONFRAMEWORK"))
# unix-style shared library enabled
SHARED = bool(get_config_var("Py_ENABLE_SHARED"))

print("implementation", platform.python_implementation())
implementation = platform.python_implementation()
if sys.implementation.name == "nogil":
implementation = "NoGIL"

print("implementation", implementation)
print("version_major", sys.version_info[0])
print("version_minor", sys.version_info[1])
print("shared", PYPY or ANACONDA or WINDOWS or FRAMEWORK or SHARED)
Expand Down Expand Up @@ -639,6 +647,7 @@ impl FromStr for PythonVersion {
pub enum PythonImplementation {
CPython,
PyPy,
NoGIL,
}

impl PythonImplementation {
Expand All @@ -653,6 +662,8 @@ impl PythonImplementation {
Ok(PythonImplementation::PyPy)
} else if soabi.starts_with("cpython") {
Ok(PythonImplementation::CPython)
} else if soabi.starts_with("nogil") {
Ok(PythonImplementation::NoGIL)
} else {
bail!("unsupported Python interpreter");
}
Expand All @@ -664,6 +675,7 @@ impl Display for PythonImplementation {
match self {
PythonImplementation::CPython => write!(f, "CPython"),
PythonImplementation::PyPy => write!(f, "PyPy"),
PythonImplementation::NoGIL => write!(f, "NoGIL"),
}
}
}
Expand All @@ -674,6 +686,7 @@ impl FromStr for PythonImplementation {
match s {
"CPython" => Ok(PythonImplementation::CPython),
"PyPy" => Ok(PythonImplementation::PyPy),
"NoGIL" => Ok(PythonImplementation::NoGIL),
_ => bail!("unknown interpreter: {}", s),
}
}
Expand Down Expand Up @@ -1523,7 +1536,7 @@ fn default_lib_name_unix(
ld_version: Option<&str>,
) -> String {
match implementation {
PythonImplementation::CPython => match ld_version {
PythonImplementation::CPython | PythonImplementation::NoGIL => match ld_version {
Some(ld_version) => format!("python{}", ld_version),
None => {
if version > PythonVersion::PY37 {
Expand Down
29 changes: 29 additions & 0 deletions pyo3-ffi/src/dictobject.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ extern "C" {
#[cfg_attr(PyPy, link_name = "PyPyDict_GetItem")]
pub fn PyDict_GetItem(mp: *mut PyObject, key: *mut PyObject) -> *mut PyObject;
pub fn PyDict_GetItemWithError(mp: *mut PyObject, key: *mut PyObject) -> *mut PyObject;
#[cfg_attr(Py_NOGIL, link_name = "PyDict_GetItemWithError2")]
pub fn _PyDict_FetchItemWithError(mp: *mut PyObject, key: *mut PyObject) -> *mut PyObject;
#[cfg_attr(PyPy, link_name = "PyPyDict_SetItem")]
pub fn PyDict_SetItem(mp: *mut PyObject, key: *mut PyObject, item: *mut PyObject) -> c_int;
#[cfg_attr(PyPy, link_name = "PyPyDict_DelItem")]
Expand Down Expand Up @@ -67,6 +69,33 @@ extern "C" {
// skipped 3.10 / ex-non-limited PyObject_GenericGetDict
}

#[inline]
#[cfg(not(Py_NOGIL))]
pub unsafe fn PyDict_FetchItem(mp: *mut PyObject, key: *mut PyObject) -> *mut PyObject {
_Py_XNewRef(PyDict_GetItem(mp, key))
}

#[inline]
#[cfg(Py_NOGIL)]
pub unsafe fn PyDict_FetchItem(mp: *mut PyObject, key: *mut PyObject) -> *mut PyObject {
use crate::PyErr_Clear;
let obj = _PyDict_FetchItemWithError(mp, key);
PyErr_Clear();
obj
}

#[inline]
#[cfg(not(Py_NOGIL))]
pub unsafe fn PyDict_FetchItemWithError(mp: *mut PyObject, key: *mut PyObject) -> *mut PyObject {
_Py_XNewRef(PyDict_GetItemWithError(mp, key))
}

#[inline]
#[cfg(Py_NOGIL)]
pub unsafe fn PyDict_FetchItemWithError(mp: *mut PyObject, key: *mut PyObject) -> *mut PyObject {
_PyDict_FetchItemWithError(mp, key)
}

#[cfg_attr(windows, link(name = "pythonXY"))]
extern "C" {
pub static mut PyDictKeys_Type: PyTypeObject;
Expand Down
14 changes: 14 additions & 0 deletions pyo3-ffi/src/listobject.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ extern "C" {
pub fn PyList_Size(arg1: *mut PyObject) -> Py_ssize_t;
#[cfg_attr(PyPy, link_name = "PyPyList_GetItem")]
pub fn PyList_GetItem(arg1: *mut PyObject, arg2: Py_ssize_t) -> *mut PyObject;
#[cfg_attr(Py_NOGIL, link_name = "PyList_Item")]
pub fn _PyList_FetchItem(arg1: *mut PyObject, arg2: Py_ssize_t) -> *mut PyObject;
#[cfg_attr(PyPy, link_name = "PyPyList_SetItem")]
pub fn PyList_SetItem(arg1: *mut PyObject, arg2: Py_ssize_t, arg3: *mut PyObject) -> c_int;
#[cfg_attr(PyPy, link_name = "PyPyList_Insert")]
Expand Down Expand Up @@ -64,3 +66,15 @@ extern "C" {
#[cfg_attr(PyPy, link_name = "PyPyList_SET_ITEM")]
pub fn PyList_SET_ITEM(arg1: *mut PyObject, arg2: Py_ssize_t, arg3: *mut PyObject);
}

#[inline]
#[cfg(not(Py_NOGIL))]
pub unsafe fn PyList_FetchItem(list: *mut PyObject, index: Py_ssize_t) -> *mut PyObject {
_Py_XNewRef(PyList_GetItem(list, index))
}

#[inline]
#[cfg(Py_NOGIL)]
pub unsafe fn PyList_FetchItem(list: *mut PyObject, index: Py_ssize_t) -> *mut PyObject {
_PyList_FetchItem(list, index)
}
Comment on lines +70 to +80
Copy link
Member

@davidhewitt davidhewitt Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

TBH, adding these provisional APIs and changing PyO3 to use them is a 👎 from me, the callsites which you modified should already be converting borrowed references to owned, and if they didn't, that's a separate bug.

EDIT: I reread the PEP and see now that it's necessary for thread-safety that the borrowed-to-owned conversion is done within the interpreter rather than PyO3. So I guess this would indeed be necessary.

Copy link
Author

Choose a reason for hiding this comment

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

The idea behind this proposed change is that, when running without the GIL, the retrieval from list and the acquisition of an owned reference needs to be performed atomically in relation to any concurrent modifications. The PEP 703 proposes PyList_FetchItem and PyDict_GetItem which have this behavior -- they return owned references and are safe in the face of concurrent modification.

54 changes: 42 additions & 12 deletions pyo3-ffi/src/object.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ pub const PyObject_HEAD_INIT: PyObject = PyObject {
_ob_next: std::ptr::null_mut(),
#[cfg(py_sys_config = "Py_TRACE_REFS")]
_ob_prev: std::ptr::null_mut(),
#[cfg(Py_NOGIL)]
ob_tid: 0,
#[cfg(Py_NOGIL)]
ob_reflocal: 1,
#[cfg(Py_NOGIL)]
ob_ref_shared: 0,
#[cfg(not(Py_NOGIL))]
ob_refcnt: 1,
#[cfg(PyPy)]
ob_pypy_link: 0,
Expand All @@ -35,6 +42,13 @@ pub struct PyObject {
pub _ob_next: *mut PyObject,
#[cfg(py_sys_config = "Py_TRACE_REFS")]
pub _ob_prev: *mut PyObject,
#[cfg(Py_NOGIL)]
pub ob_tid: usize,
#[cfg(Py_NOGIL)]
pub ob_reflocal: u32,
#[cfg(Py_NOGIL)]
pub ob_ref_shared: u32,
#[cfg(not(Py_NOGIL))]
pub ob_refcnt: Py_ssize_t,
#[cfg(PyPy)]
pub ob_pypy_link: Py_ssize_t,
Expand Down Expand Up @@ -62,11 +76,19 @@ pub unsafe fn Py_Is(x: *mut PyObject, y: *mut PyObject) -> c_int {
// skipped _Py_REFCNT: defined in Py_REFCNT

#[inline]
#[cfg(not(Py_NOGIL))]
pub unsafe fn Py_REFCNT(ob: *mut PyObject) -> Py_ssize_t {
assert!(!ob.is_null());
(*ob).ob_refcnt
}

#[inline]
#[cfg(Py_NOGIL)]
pub unsafe fn Py_REFCNT(ob: *mut PyObject) -> Py_ssize_t {
assert!(!ob.is_null());
Py_RefCnt(ob)
}

#[inline]
pub unsafe fn Py_TYPE(ob: *mut PyObject) -> *mut PyTypeObject {
(*ob).ob_type
Expand Down Expand Up @@ -409,26 +431,32 @@ extern "C" {

// Reference counting macros.
#[inline]
#[cfg(not(any(py_sys_config = "Py_REF_DEBUG", Py_NOGIL)))]
pub unsafe fn Py_INCREF(op: *mut PyObject) {
if cfg!(py_sys_config = "Py_REF_DEBUG") {
Py_IncRef(op)
} else {
(*op).ob_refcnt += 1
}
(*op).ob_refcnt += 1
}

#[inline]
#[cfg(any(py_sys_config = "Py_REF_DEBUG", Py_NOGIL))]
pub unsafe fn Py_INCREF(op: *mut PyObject) {
Py_IncRef(op)
Copy link
Member

Choose a reason for hiding this comment

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

@colesbury - I understood that for the CPython stable API there was a discussion about making refcounting details internal to CPython but it was deemed infeasible due to performance regression.

Here this patch seems to do exactly that. Are you able to comment on the estimated performance impact for extensions by changing refcounting to be through an FFI call?

Copy link
Author

Choose a reason for hiding this comment

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

I think I have heard something similar, but I can't find the original source and don't know for which extensions it was deemed too great a cost.

There didn't seem to be an noticeable performance impact for the cryptography extension, but I don't know enough to make any general estimates.

}

#[inline]
#[cfg(not(any(py_sys_config = "Py_REF_DEBUG", Py_NOGIL)))]
pub unsafe fn Py_DECREF(op: *mut PyObject) {
if cfg!(py_sys_config = "Py_REF_DEBUG") {
Py_DecRef(op)
} else {
(*op).ob_refcnt -= 1;
if (*op).ob_refcnt == 0 {
_Py_Dealloc(op)
}
(*op).ob_refcnt -= 1;
if (*op).ob_refcnt == 0 {
_Py_Dealloc(op)
}
}

#[inline]
#[cfg(any(py_sys_config = "Py_REF_DEBUG", Py_NOGIL))]
pub unsafe fn Py_DECREF(op: *mut PyObject) {
Py_DecRef(op)
}

#[inline]
pub unsafe fn Py_CLEAR(op: *mut *mut PyObject) {
let tmp = *op;
Expand Down Expand Up @@ -457,6 +485,8 @@ extern "C" {
pub fn Py_IncRef(o: *mut PyObject);
#[cfg_attr(PyPy, link_name = "PyPy_DecRef")]
pub fn Py_DecRef(o: *mut PyObject);
#[cfg(Py_NOGIL)]
pub fn Py_RefCnt(o: *mut PyObject) -> Py_ssize_t;

#[cfg(Py_3_10)]
pub fn Py_NewRef(obj: *mut PyObject) -> *mut PyObject;
Expand Down
8 changes: 4 additions & 4 deletions src/types/dict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,10 @@ impl PyDict {
K: ToPyObject,
{
unsafe {
let ptr = ffi::PyDict_GetItem(self.as_ptr(), key.to_object(self.py()).as_ptr());
let ptr = ffi::PyDict_FetchItem(self.as_ptr(), key.to_object(self.py()).as_ptr());
NonNull::new(ptr).map(|p| {
// PyDict_GetItem return s borrowed ptr, must make it owned for safety (see #890).
self.py().from_owned_ptr(ffi::_Py_NewRef(p.as_ptr()))
self.py().from_owned_ptr(p.as_ptr())
})
}
}
Expand All @@ -166,12 +166,12 @@ impl PyDict {
{
unsafe {
let ptr =
ffi::PyDict_GetItemWithError(self.as_ptr(), key.to_object(self.py()).as_ptr());
ffi::PyDict_FetchItemWithError(self.as_ptr(), key.to_object(self.py()).as_ptr());
if !ffi::PyErr_Occurred().is_null() {
return Err(PyErr::fetch(self.py()));
}

Ok(NonNull::new(ptr).map(|p| self.py().from_owned_ptr(ffi::_Py_NewRef(p.as_ptr()))))
Ok(NonNull::new(ptr).map(|p| self.py().from_owned_ptr(p.as_ptr())))
}
}

Expand Down
7 changes: 3 additions & 4 deletions src/types/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,7 @@ impl PyList {
/// ```
pub fn get_item(&self, index: usize) -> PyResult<&PyAny> {
unsafe {
let item = ffi::PyList_GetItem(self.as_ptr(), index as Py_ssize_t);
// PyList_GetItem return borrowed ptr; must make owned for safety (see #890).
ffi::Py_XINCREF(item);
let item = ffi::PyList_FetchItem(self.as_ptr(), index as Py_ssize_t);
self.py().from_owned_ptr_or_err(item)
}
}
Expand All @@ -141,7 +139,8 @@ impl PyList {
///
/// # Safety
///
/// Caller must verify that the index is within the bounds of the list.
/// Caller must verify that the index is within the bounds of the list and that no other
/// thread is concurrently modifying the list.
#[cfg(not(Py_LIMITED_API))]
pub unsafe fn get_item_unchecked(&self, index: usize) -> &PyAny {
let item = ffi::PyList_GET_ITEM(self.as_ptr(), index as Py_ssize_t);
Expand Down