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

Remove GILProtected on free-threaded build #4504

Merged
merged 7 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
77 changes: 77 additions & 0 deletions guide/src/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,83 @@ impl<'a, 'py> IntoPyObject<'py> for &'a MyPyObjectWrapper {
```
</details>

### Free-threaded Python Support
<details open>
<summary><small>Click to expand</small></summary>

PyO3 0.23 introduces preliminary support for the new free-threaded build of
CPython 3.13. PyO3 features that implicitly assumed the existence of the GIL
are not exposed in the free-threaded build, since they are no longer safe.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the end is GILProtected the only feature we're hiding? It looks like #4512 leaves GILOnceCell exposed.

Copy link
Member

Choose a reason for hiding this comment

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

I'm considering #4512 and #4513 as possible ideas, would welcome your thoughts on those two options. It seems like migrating off GILOnceCell is desired in the long term, but I'd prefer not force users to do that until we can support module state...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's still up in the air I'll leave this language as-is. We can massage it before the 0.23 release (which I think we're getting close to!).

Copy link
Member

Choose a reason for hiding this comment

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

Yep. I also think at some point before the release we should move some of this content into a more permanent, dedicated guide page and then make this migration entry link there. This content will be relevant going forward, not just for those upgrading.


If you make use of these features then you will need to account for the
unavailability of this API in the free-threaded build. One way to handle it is
via conditional compilation -- extensions built for the free-threaded build will
have the `Py_GIL_DISABLED` attribute defined.

### `GILProtected`

`GILProtected` allows mutable access to static data by leveraging the GIL to
lock concurrent access from other threads. In free-threaded python there is no
GIL, so you will need to replace this type with some other form of locking. In
many cases, `std::sync::Atomic` or `std::sync::Mutex` will be sufficient. If the
locks do not guard the execution of arbitrary Python code or use of the CPython
C API then conditional compilation is likely unnecessary since `GILProtected`
was not needed in the first place.

Before:

```rust,ignore
ngoldbaum marked this conversation as resolved.
Show resolved Hide resolved
# use pyo3::prelude::*;
use pyo3::sync::GILProtected;
use pyo3::types::{PyDict, PyNone};
use std::cell::RefCell;

static OBJECTS: GILProtected<RefCell<Vec<Py<PyDict>>>> =
GILProtected::new(RefCell::new(Vec::new()));

fn main() {
Python::with_gil(|py: Python| {
let d = PyDict::new(py);
// stand-in for something that executes arbitrary python code
d.set_item(PyNone::get(py), PyNone::get(py)).unwrap();
OBJECTS.get(py).borrow_mut().push(d.unbind());
});
}
```

After:

```rust
use pyo3::prelude::*;
#[cfg(not(Py_GIL_DISABLED))]
use pyo3::sync::GILProtected;
use pyo3::types::{PyDict, PyNone};
#[cfg(not(Py_GIL_DISABLED))]
use std::cell::RefCell;
#[cfg(Py_GIL_DISABLED)]
use std::sync::Mutex;

#[cfg(not(Py_GIL_DISABLED))]
static OBJECTS: GILProtected<RefCell<Vec<Py<PyDict>>>> =
GILProtected::new(RefCell::new(Vec::new()));
#[cfg(Py_GIL_DISABLED)]
static OBJECTS: Mutex<Vec<Py<PyDict>>> = Mutex::new(Vec::new());

fn main() {
Python::with_gil(|py| {
let d = PyDict::new(py);
// stand-in for something that executes arbitrary python code
d.set_item(PyNone::get(py), PyNone::get(py)).unwrap();
#[cfg(not(Py_GIL_DISABLED))]
OBJECTS.get(py).borrow_mut().push(d.unbind());
#[cfg(Py_GIL_DISABLED)]
OBJECTS.lock().unwrap().push(d.unbind());
});
}
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll expand this to include a discussion of how to handle executing arbitrary python code while you hold the global lock by leveraging PyMutex in a followup.

</details>

## from 0.21.* to 0.22

### Deprecation of `gil-refs` feature continues
Expand Down
2 changes: 2 additions & 0 deletions newsfragments/4504.changed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
* The `GILProtected` struct is not available on the free-threaded build of
Python 3.13.
24 changes: 13 additions & 11 deletions src/impl_/pyclass/lazy_type_object.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use std::{
cell::RefCell,
ffi::CStr,
marker::PhantomData,
thread::{self, ThreadId},
Expand All @@ -11,11 +10,13 @@ use crate::{
impl_::pyclass::MaybeRuntimePyMethodDef,
impl_::pymethods::PyMethodDefType,
pyclass::{create_type_object, PyClassTypeObject},
sync::{GILOnceCell, GILProtected},
sync::GILOnceCell,
types::PyType,
Bound, PyClass, PyErr, PyObject, PyResult, Python,
};

use std::sync::Mutex;

use super::PyClassItemsIter;

/// Lazy type object for PyClass.
Expand All @@ -27,7 +28,7 @@ struct LazyTypeObjectInner {
value: GILOnceCell<PyClassTypeObject>,
// Threads which have begun initialization of the `tp_dict`. Used for
// reentrant initialization detection.
initializing_threads: GILProtected<RefCell<Vec<ThreadId>>>,
initializing_threads: Mutex<Vec<ThreadId>>,
tp_dict_filled: GILOnceCell<()>,
}

Expand All @@ -38,7 +39,7 @@ impl<T> LazyTypeObject<T> {
LazyTypeObject(
LazyTypeObjectInner {
value: GILOnceCell::new(),
initializing_threads: GILProtected::new(RefCell::new(Vec::new())),
initializing_threads: Mutex::new(Vec::new()),
tp_dict_filled: GILOnceCell::new(),
},
PhantomData,
Expand Down Expand Up @@ -117,7 +118,7 @@ impl LazyTypeObjectInner {

let thread_id = thread::current().id();
{
let mut threads = self.initializing_threads.get(py).borrow_mut();
let mut threads = self.initializing_threads.lock().unwrap();
if threads.contains(&thread_id) {
// Reentrant call: just return the type object, even if the
// `tp_dict` is not filled yet.
Expand All @@ -127,20 +128,18 @@ impl LazyTypeObjectInner {
}

struct InitializationGuard<'a> {
initializing_threads: &'a GILProtected<RefCell<Vec<ThreadId>>>,
py: Python<'a>,
initializing_threads: &'a Mutex<Vec<ThreadId>>,
thread_id: ThreadId,
}
impl Drop for InitializationGuard<'_> {
fn drop(&mut self) {
let mut threads = self.initializing_threads.get(self.py).borrow_mut();
let mut threads = self.initializing_threads.lock().unwrap();
threads.retain(|id| *id != self.thread_id);
}
}

let guard = InitializationGuard {
initializing_threads: &self.initializing_threads,
py,
thread_id,
};

Expand Down Expand Up @@ -185,8 +184,11 @@ impl LazyTypeObjectInner {

// Initialization successfully complete, can clear the thread list.
// (No further calls to get_or_init() will try to init, on any thread.)
std::mem::forget(guard);
self.initializing_threads.get(py).replace(Vec::new());
let mut threads = {
drop(guard);
self.initializing_threads.lock().unwrap()
};
threads.clear();
result
});

Expand Down
10 changes: 9 additions & 1 deletion src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,21 @@
//! [PEP 703]: https://peps.python.org/pep-703/
use crate::{
types::{any::PyAnyMethods, PyString, PyType},
Bound, Py, PyResult, PyVisit, Python,
Bound, Py, PyResult, Python,
};
use std::cell::UnsafeCell;

#[cfg(not(Py_GIL_DISABLED))]
use crate::PyVisit;

/// Value with concurrent access protected by the GIL.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should document that this type is unavailable on the freethreaded build and recommend use of atomics or Mutex as necessary. I guess in the future if we offer PyMutex as a type then we can recommend here too (as I understood it, if it blocks it will also release the GIL to help avoid deadlocking).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll take a look at adding and documenting safe wrappers for PyMutex that pyo3 can expose in a followup.

///
/// This is a synchronization primitive based on Python's global interpreter lock (GIL).
/// It ensures that only one thread at a time can access the inner value via shared references.
/// It can be combined with interior mutability to obtain mutable references.
///
/// This type is not defined for extensions built against the free-threaded CPython ABI.
///
/// # Example
///
/// Combining `GILProtected` with `RefCell` enables mutable access to static data:
Expand All @@ -31,10 +36,12 @@ use std::cell::UnsafeCell;
/// NUMBERS.get(py).borrow_mut().push(42);
/// });
/// ```
#[cfg(not(Py_GIL_DISABLED))]
ngoldbaum marked this conversation as resolved.
Show resolved Hide resolved
pub struct GILProtected<T> {
value: T,
}

#[cfg(not(Py_GIL_DISABLED))]
impl<T> GILProtected<T> {
/// Place the given value under the protection of the GIL.
pub const fn new(value: T) -> Self {
Expand All @@ -52,6 +59,7 @@ impl<T> GILProtected<T> {
}
}

#[cfg(not(Py_GIL_DISABLED))]
unsafe impl<T> Sync for GILProtected<T> where T: Send {}

/// A write-once cell similar to [`once_cell::OnceCell`](https://docs.rs/once_cell/latest/once_cell/).
Expand Down
Loading