Initial free threaded bindings #4421

ngoldbaum · 2024-08-06T22:40:19Z

Makes nox -s ffi-check pass on the free-threaded build
Adds a build config visible to PyO3 for the free-threaded build
Adds bindings for PyMutex (see https://docs.python.org/3.13/c-api/init.html#c.PyMutex)
Adds initial CI config with continue_on_error set for the test run.

ngoldbaum · 2024-08-06T22:45:36Z

noxfile.py

@@ -32,6 +33,7 @@
 PYO3_DOCS_TARGET = PYO3_TARGET / "doc"
 PY_VERSIONS = ("3.7", "3.8", "3.9", "3.10", "3.11", "3.12", "3.13")
 PYPY_VERSIONS = ("3.7", "3.8", "3.9", "3.10")
+IS_FREE_THREADED = bool(sysconfig.get_config_var("Py_GIL_DISABLED"))


not sure this is the "correct" way to do this in nox

So this makes it conditional on the host python, but really what we care about is the target python, I think?

I think as far as our use of nox is concerned, these are the same. Is there an alternative?

It looks like the reason this was added is because we made the abi3 build on free-threaded a hard failure. Maybe we should relax that and then we don't need a change in the noxfile for free-threading?

ngoldbaum · 2024-08-06T22:46:02Z

pyo3-build-config/src/impl_.rs

+ "cargo:rustc-cfg=Py_3_12".to_owned(),
+ "cargo:rustc-cfg=Py_3_13".to_owned(),
+ "cargo:rustc-cfg=Py_GIL_DISABLED".to_owned(),
+ "cargo:rustc-cfg=py_sys_config=\"Py_GIL_DISABLED\"".to_owned(),


should we define both configs in this case? Or only the top-level Py_GIL_DISABLED one?

I think let's go for just the top-level Py_GIL_DISABLED 👍. (You'll need to special-case this when printing the BuildFlags. as cfgs, I guess)

Please document in pyo3-build-config/lib.rs (in the docstring for use_pyo3_cfgs).

ngoldbaum · 2024-08-06T22:47:30Z

pyo3-ffi/src/object.rs

+ return _Py_IMMORTAL_REFCNT;
+ }
+ let shared = (AtomicIsize::from((*ob).ob_ref_shared)).load(Relaxed);
+ local as Py_ssize_t + Py_ssize_t::from(shared >> _Py_REF_SHARED_SHIFT)


This is the only kind of hairy bit, since there's no function exposed in the API to get the refcount

This is my first time dealing with rust atomics. I have no idea if it's safe to create them inline and then do an atomic load like this.

AtomicU32::from((*ob).ob_ref_local) creates a new atomic whose changes will not be reflected in the original ob_ref_local. So this is definitely wrong. Atomic::from_ptr would be more appropriate.

Anyway I would feel better if we just used the C functions to do refcounting, similar to #4311 (comment). I'm sure @davidhewitt will have thoughts on this.

Unfortunately there is no function for this in the API, only a static inline function.

In #3357 we discussed maybe we should just remove this API.

I think we still might need an implementation though if we need to expose it in the FFI for users of PyO3, e.g. as @davidhewitt implies in #3357 (comment).

My understanding is that the plan for 3.14 is to either expose a function for this or to expose a function that provides similar functionality without needing to expose refcounts directly. Something like Py_IsOnlyReference for refcount==1 and otherwise everything else becomes an implementation detail.

Unfortunately that doesn't help us for 3.13.

I think that we probably do need to keep this API exposed (even after #4065 we use it for debugging and I think others might want to too). I guess that given it's a static inline function, let's just go down the Atomic::from_ptr route and hope that CPython can provide a C function in 3.14.

mejrs · 2024-08-07T09:20:27Z

pyo3-ffi/src/cpython/lock.rs

+#[repr(C)]
+#[derive(Debug, Copy, Clone)]
+pub struct PyMutex {
+ pub _bits: u8,
+}


It'd be wrong to derive Copy and/or Clone, the documentation explicitly says it should not be moved.

The Debug impl + not using atomics means the debug impl would race.

The field is also not public in the C api, I'm inclined to keep it that way here as well.

Suggested change

#[repr(C)]

#[derive(Debug, Copy, Clone)]

pub struct PyMutex {

pub _bits: u8,

}

#[repr(C)]

#[derive(Default)]

pub struct PyMutex {

_bits: u8,

_pin: PhantomPinned,

}

Unfortunately that won't work because PyObject needs those traits. At least if we want to say that ob_mutex is a PyObject as I have it in this version of the PR. Maybe better to write the PyObject bindings with ob_mutex as a u8. It's an internal, private CPython field and we wouldn't be using it otherwise.

I think we should probably remove Copy & Clone from PyObject in that case, those traits sound to me like an accident waiting to happen anyway 👍

The Default implementation seems a bit risky to me because of the existence of std::mem::take. I'm not sure _pin will save us from that unless we don't give users a way to get at &mut PyMutex, which e.g. they could just do with:

let mut m = PyMutex::default(); let moved = std::mem::take(&mut mutex);

This would also be a move-related bug:

let mut m = PyMutex::default(); PyMutex_Lock(&mut m); let moved = m;

If we want to allow users to construct a PyMutex, I think we need to remove the Default implementation and have something like

impl PyMutex { pub fn new() -> Pin<Box<PyMutex>> }

... but I also wonder whether that's overkill and we should just

not bother with _pin, and

not give users a way to construct PyMutex for now (we will need it inside the crate for PyObject_HEAD_INIT, but we can just make pub(crate) _bits).

I'm not sure _pin will save us

The PhantomPinned itself doesn't do anything. It lets us build safe abstractions on top of it.

If we want to allow users to construct a PyMutex

We can remove the Default impl and document that users can create it with mem::zeroed, which is what CPython's documentation does.

This would also be a move-related bug:

A safe api would probably do something like this (to follow the example of having a PyMutex on the stack), which is possible because of the PhantomPinned.

let mutex: Pin<&mut PyMutex> = pin!(PyMutex { _bits: 0, _pin: PhantomPinned }); lock(mutex.as_ref());

Unfortunately that won't work because PyObject needs those traits

@ngoldbaum we can still have a Debug impl, it just needs a manual impl that correctly accesses the PyMutex value (or, ideally it should just be an AtomicU8).

I agree with removing the Copy and Clone impls.

alex · 2024-08-07T11:45:19Z

noxfile.py

@@ -32,6 +33,7 @@
 PYO3_DOCS_TARGET = PYO3_TARGET / "doc"
 PY_VERSIONS = ("3.7", "3.8", "3.9", "3.10", "3.11", "3.12", "3.13")
 PYPY_VERSIONS = ("3.7", "3.8", "3.9", "3.10")
+IS_FREE_THREADED = bool(sysconfig.get_config_var("Py_GIL_DISABLED"))


So this makes it conditional on the host python, but really what we care about is the target python, I think?

alex · 2024-08-07T11:45:55Z

pyo3-build-config/src/impl_.rs

+ if build_flags.0.contains(&BuildFlag::Py_GIL_DISABLED) && abi3 {
+ bail!("Cannot set Py_LIMITED_API and Py_GIL_DISABLED at the same time")
+ }


I think in some cases we simply warn and disable Py_LIMITED_API (e.g., PyPy), do we have a principled reason to do one vs. the other?

In 3.13 Py_GIL_DISABLED implies Py_LIMITED_API isn't set and I wanted a way to enforce that. Warning and disabling the limited API gives the same effect.

I see there's early-exits in some of these functions for pypy and graal. I think can do something similar for free-threaded.

alex · 2024-08-07T11:48:08Z

pyo3-ffi/src/object.rs

+ return _Py_IMMORTAL_REFCNT;
+ }
+ let shared = (AtomicIsize::from((*ob).ob_ref_shared)).load(Relaxed);
+ local as Py_ssize_t + Py_ssize_t::from(shared >> _Py_REF_SHARED_SHIFT)


In #3357 we discussed maybe we should just remove this API.

davidhewitt

Thanks for helping out with this! Clearly some sticky cases, overall this is looking great already :)

davidhewitt · 2024-08-09T15:07:55Z

pyo3-ffi/src/object.rs

+ return _Py_IMMORTAL_REFCNT;
+ }
+ let shared = (AtomicIsize::from((*ob).ob_ref_shared)).load(Relaxed);
+ local as Py_ssize_t + Py_ssize_t::from(shared >> _Py_REF_SHARED_SHIFT)


I think that we probably do need to keep this API exposed (even after #4065 we use it for debugging and I think others might want to too). I guess that given it's a static inline function, let's just go down the Atomic::from_ptr route and hope that CPython can provide a C function in 3.14.

davidhewitt · 2024-08-09T15:24:06Z

pyo3-ffi/src/cpython/lock.rs

+#[repr(C)]
+#[derive(Debug, Copy, Clone)]
+pub struct PyMutex {
+ pub _bits: u8,
+}


The Default implementation seems a bit risky to me because of the existence of std::mem::take. I'm not sure _pin will save us from that unless we don't give users a way to get at &mut PyMutex, which e.g. they could just do with:

let mut m = PyMutex::default(); let moved = std::mem::take(&mut mutex);

This would also be a move-related bug:

let mut m = PyMutex::default(); PyMutex_Lock(&mut m); let moved = m;

If we want to allow users to construct a PyMutex, I think we need to remove the Default implementation and have something like

impl PyMutex { pub fn new() -> Pin<Box<PyMutex>> }

... but I also wonder whether that's overkill and we should just

not bother with _pin, and

not give users a way to construct PyMutex for now (we will need it inside the crate for PyObject_HEAD_INIT, but we can just make pub(crate) _bits).

pyo3-ffi/src/cpython/weakrefobject.rs

pyo3-ffi/src/object.rs

davidhewitt · 2024-08-09T15:29:24Z

pyo3-ffi/src/object.rs

@@ -507,8 +557,13 @@ extern "C" {

 #[inline(always)]
 pub unsafe fn Py_INCREF(op: *mut PyObject) {


Potentially can we leave a TODO or fixme to use inline reference counting on freethreaded builds? (Here and in Py_DECREF.)

If it helps us eke out a tiny bit of performance, it might be a nice thing to do eventually.

davidhewitt · 2024-08-09T15:32:28Z

noxfile.py

@@ -32,6 +33,7 @@
 PYO3_DOCS_TARGET = PYO3_TARGET / "doc"
 PY_VERSIONS = ("3.7", "3.8", "3.9", "3.10", "3.11", "3.12", "3.13")
 PYPY_VERSIONS = ("3.7", "3.8", "3.9", "3.10")
+IS_FREE_THREADED = bool(sysconfig.get_config_var("Py_GIL_DISABLED"))


I think as far as our use of nox is concerned, these are the same. Is there an alternative?

It looks like the reason this was added is because we made the abi3 build on free-threaded a hard failure. Maybe we should relax that and then we don't need a change in the noxfile for free-threading?

davidhewitt · 2024-08-09T15:37:03Z

pyo3-build-config/src/impl_.rs

+ "cargo:rustc-cfg=Py_3_12".to_owned(),
+ "cargo:rustc-cfg=Py_3_13".to_owned(),
+ "cargo:rustc-cfg=Py_GIL_DISABLED".to_owned(),
+ "cargo:rustc-cfg=py_sys_config=\"Py_GIL_DISABLED\"".to_owned(),


I think let's go for just the top-level Py_GIL_DISABLED 👍. (You'll need to special-case this when printing the BuildFlags. as cfgs, I guess)

Please document in pyo3-build-config/lib.rs (in the docstring for use_pyo3_cfgs).

mejrs · 2024-08-09T17:35:20Z

I've been giving it some thought and I think we really should use atomic integers (in PyMutex and the relevant refcount fields).

The code to correctly access them is rather error prone. Suppose a user wants to read the vvalue of a PyObject's PyMutex field.

They will have to write (and figure out that this is how they have to write it):

let x: *mut PyObject= ...;
let value = AtomicU8::from_ptr(addr_of_mut!((*x).ob_mutex)).load(Ordering::Relaxed);

rather than

let value = *x.ob_mutex;

and if they want to write to it they must also avoid creating mutable references, which is another poorly known footgun.

ngoldbaum · 2024-08-09T17:40:49Z

@colesbury I suspect you might have an opinion about how to wrap the new PyObject fields in PyO3 and whether or not using rust atomics in the PyObject bindings makes sense.

colesbury · 2024-08-09T17:59:50Z

I've been giving it some thought and I think we really should use atomic integers (in PyMutex and the relevant refcount fields)

Yeah, I think using atomic integers for the fields is a good idea.

I'm not sure if there's much value for exposing PyMutex in PyO3. The ob_mutex field shouldn't be locked directly. If you want to lock a Python object, you should use the critical section API, which takes a PyObject* argument.

For more generic locking, you can use PyMutex or Rust locking primitives. The only advantage of PyMutex is that it saves the thread-state before blocking if it needs to wait.

davidhewitt · 2024-08-09T18:52:09Z

Note that using atomic values for the fields probably means we want to consider portable_atomic as per #3619, but I think that crate uses a global lock as a fallback which is actually unsound here given the FFI.

Alternatively on platforms where std doesn't have atomic types we could just define our own opaque wrapper which users cannot access.

ngoldbaum · 2024-08-09T19:02:28Z

I think CPython requires atomics support. Are there use-cases for a version of pyo3 on hosts where CPython can't be compiled?

colesbury · 2024-08-09T19:03:20Z

Are there platforms where std doesn't have any atomic types? #3614 sounds like an issue using 64-bit atomic field on a 32-bit PowerPC. None of the PyObject fields are 64-bits on 32-bit platforms.

davidhewitt · 2024-08-09T20:37:30Z

That's a good question; I wouldn't know the answer to that without looking through the rust source tree (and maybe others could define their own platforms). In practice it may not be a concern we have to worry about for these cases. We could punt on worrying about that until someone actually reports an issue for their platform, I guess 👀

davidhewitt · 2024-08-09T20:38:24Z

I think CPython requires atomics support. Are there use-cases for a version of pyo3 on hosts where CPython can't be compiled?

Ah just seen this - in that case I think we can assume using atomics here is fine 👍

Co-authored-by: David Hewitt <[email protected]>

ngoldbaum · 2024-08-13T22:56:42Z

It turns out there's one more issue adding PyMutex to the FFI bindings reveals: PyObject_HEAD_INIT should maybe be a function instead of a constant? It being a constant is problematic if one of the fields has interior mutability.

ngoldbaum · 2024-08-14T19:37:37Z

It turns out, for whatever reason, only a debug free-threaded python build successfully runs the pyo3 tests, at least on my Mac's development environment using pyenv. See python/cpython#122918 (comment). Just sharing in case anyone else wants to try this out and sees mysterious crashes with tracebacks that include Py_InitializeEx.

colesbury · 2024-08-14T19:39:42Z

pyo3-ffi/src/object.rs

+ #[cfg(Py_GIL_DISABLED)]
+ ob_gc_bits: 0,
+ #[cfg(Py_GIL_DISABLED)]
+ ob_ref_local: AtomicU32::new(0),


This should be immortal (u32::MAX) I think

colesbury · 2024-08-14T19:52:50Z

pyo3-ffi/src/cpython/lock.rs

+#[repr(C)]
+pub struct PyMutex {
+ _bits: AtomicU8,
+}


Looking at Rust's built-in mutex types, I think we'd want a constructor like:

impl PyMutex { pub const fn new() -> PyMutex { PyMutex { _bits: AtomicU8::new(0) } } }

colesbury · 2024-08-14T19:53:49Z

pyo3-ffi/src/object.rs

+ #[cfg(Py_GIL_DISABLED)]
+ _padding: 0,
+ #[cfg(Py_GIL_DISABLED)]
+ ob_mutex: unsafe { mem::zeroed::<PyMutex>() },


I think this can be PyMutex::new() if we define it as above.

That avoids the unsafe block. And maybe allows PyObject_HEAD_INIT to remain a constant? Are there other fields with interior mutability?

ngoldbaum · 2024-08-14T19:59:17Z

Hmm, maybe this is what @davidhewitt is referring to above in #4421 (comment) with the bit about pub(crate) _bits. Let me see if I can get that working...

ngoldbaum · 2024-08-14T20:56:22Z

It looks like the lint triggers because of the atomic fields, so declaring the PyMutex field as pub(crate) is related but unfortunately doesn't fix the complaint about interior mutability.

mejrs · 2024-08-14T21:01:12Z

Can we not just silence the lint? The copy is intentional.

ngoldbaum mentioned this pull request Aug 6, 2024

Tracking issue for no-gil/freethreaded work #4265

Open

11 tasks

davidhewitt added the CI-build-full label Aug 6, 2024

ngoldbaum commented Aug 6, 2024

View reviewed changes

mejrs reviewed Aug 7, 2024

View reviewed changes

alex reviewed Aug 7, 2024

View reviewed changes

davidhewitt reviewed Aug 9, 2024

View reviewed changes

ngoldbaum mentioned this pull request Aug 12, 2024

Remove Copy and Clone derives from PyObject #4434

Merged

ngoldbaum and others added 15 commits August 13, 2024 14:50

add support in pyo3-build-config for free-threaded python

2685691

update object.h bindings for free-threaded build

e8613d5

Add PyMutex bindings

eeac20c

fix po3-ffi-check with free-threaded build

8eebe43

error when building with limited api and Py_GIL_DISABLED

9e9d77e

Add CI job for free-threaded build

79dd1a7

fix issues building on older pythons

5aaa03a

ci config fixup

f4219b3

fix clippy on gil-enabled 3.13 build

4ff9a62

Apply suggestions from code review

57e7e3d

Co-authored-by: David Hewitt <[email protected]>

make PyMutex and PyObject refcounting fields atomics

a042e56

add new field on PyConfig in 3.13 debug ABI

7c493eb

warn and disable abi3 on gil-disabled build

da3e50c

fix conditional compilation for PyMutex usage

77b4d96

temporarily skip test that deadlocks

96dd13e

remove Py_GIL_DISABLED from py_sys_config cfg options

dbcb9d2

ngoldbaum force-pushed the free-threaded-bindings branch from 37a21bb to dbcb9d2 Compare August 13, 2024 22:55

only expose PyMutex in 3.13

d9dac3c

bschoenmaeckers mentioned this pull request Aug 14, 2024

Add PyCriticalSection lock to Dict iterator #4439

Draft

make PyObject_HEAD_INIT a function

9b70b07

colesbury reviewed Aug 14, 2024

View reviewed changes

ngoldbaum added 3 commits August 14, 2024 13:46

intialize ob_ref_local to _Py_IMMORTAL_REFCNT_LOCAL in HEAD_INIT

3e03dd1

Fix clippy lint about static with interior mutability

f695203

add TODO comments about INCREF and DECREF in free-threaded build

848c793

colesbury reviewed Aug 14, 2024

View reviewed changes

ngoldbaum added 2 commits August 14, 2024 14:11

make the _bits field of PyMutex pub(crate)

6e66cb0

refactor so HEAD_INIT remains a constant

126c97b

ngoldbaum force-pushed the free-threaded-bindings branch from 126c97b to 4f261c2 Compare August 14, 2024 20:54

ignore clippy lint about interior mutability

162a65e

ngoldbaum force-pushed the free-threaded-bindings branch from 4f261c2 to 162a65e Compare August 14, 2024 21:07

ngoldbaum added 2 commits August 14, 2024 16:37

revert unnecessary changes to pyo3-build-config

6103d26

add changelog entries

8a9ef2d

		@@ -507,8 +557,13 @@ extern "C" {

		#[inline(always)]
		pub unsafe fn Py_INCREF(op: *mut PyObject) {

Initial free threaded bindings #4421

Are you sure you want to change the base?

Initial free threaded bindings #4421

Conversation

ngoldbaum commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mejrs Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngoldbaum Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mejrs commented Aug 9, 2024

ngoldbaum commented Aug 9, 2024

colesbury commented Aug 9, 2024

davidhewitt commented Aug 9, 2024

ngoldbaum commented Aug 9, 2024 • edited Loading

colesbury commented Aug 9, 2024

davidhewitt commented Aug 9, 2024

davidhewitt commented Aug 9, 2024

ngoldbaum commented Aug 13, 2024

ngoldbaum commented Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngoldbaum commented Aug 14, 2024 • edited Loading

ngoldbaum commented Aug 14, 2024 • edited Loading

mejrs commented Aug 14, 2024

mejrs Aug 7, 2024 •

edited

Loading

ngoldbaum Aug 8, 2024 •

edited

Loading

ngoldbaum commented Aug 9, 2024 •

edited

Loading

ngoldbaum commented Aug 14, 2024 •

edited

Loading

ngoldbaum commented Aug 14, 2024 •

edited

Loading

ngoldbaum commented Aug 14, 2024 •

edited

Loading