Initial free threaded bindings by ngoldbaum · Pull Request #4421 · PyO3/pyo3

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.

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

+        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.

davidhewitt

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

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 <mail@davidhewitt.dev>

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

+        #[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

+#[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

+        #[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.

mejrs · 2024-08-14T21:36:14Z

+impl fmt::Debug for PyMutex {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("PyMutex").finish()
+    }
+}


This can be a #[derive(Debug)] now that the field is an atomic.

davidhewitt

Overall this looks really good, and I think we're close! Thanks again 🚀

davidhewitt · 2024-08-15T20:36:47Z

+    let local = (*ob).ob_ref_local.load(Relaxed);
+    if local == _Py_IMMORTAL_REFCNT_LOCAL {
+        return _Py_IMMORTAL_REFCNT;
+    }
+    let shared = (*ob).ob_ref_shared.load(Relaxed);


I checked the CPython source to verify these are indeed relaxed loads 👍

davidhewitt · 2024-08-15T21:05:43Z

          echo PYO3_CONFIG_FILE=$PYO3_CONFIG_FILE >> $GITHUB_ENV
      - run: python3 -m nox -s test

+  test-free-threaded:


Very cool to see this job running, thanks 🙏

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

davidhewitt

Looks great to me, thanks! 🎉

ngoldbaum mentioned this pull request Aug 6, 2024

Tracking issue for no-gil/freethreaded work #4265

Closed

14 tasks

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

ngoldbaum commented Aug 6, 2024

View reviewed changes

Comment thread noxfile.py Outdated

Comment thread pyo3-build-config/src/impl_.rs Outdated

Comment thread pyo3-ffi/src/object.rs

mejrs reviewed Aug 7, 2024

View reviewed changes

Comment thread pyo3-ffi/src/cpython/lock.rs Outdated

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 <mail@davidhewitt.dev>

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

bschoenmaeckers mentioned this pull request Aug 14, 2024

Make PyDict iterator compatible with free-threaded build #4439

Merged

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

mejrs reviewed Aug 15, 2024

View reviewed changes

ngoldbaum added 2 commits August 15, 2024 11:27

use derive(Debug) for PyMutex

e1ddd01

Add PhantomPinned field to PyMutex bindings

1a088ea

davidhewitt reviewed Aug 15, 2024

View reviewed changes

ngoldbaum and others added 3 commits August 15, 2024 15:20

Update pyo3-build-config/src/impl_.rs

38bc492

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

Update pyo3-ffi/src/object.rs

a85fa4c

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

fix build config again

60c26c4

davidhewitt approved these changes Aug 15, 2024

View reviewed changes

davidhewitt added this pull request to the merge queue Aug 16, 2024

Merged via the queue into PyO3:main with commit f474810 Aug 16, 2024

ngoldbaum mentioned this pull request Apr 22, 2025

Add PyMutex::new to pyo3-ffi #5084

Closed

Conversation

ngoldbaum commented Aug 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

ngoldbaum Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mejrs commented Aug 9, 2024

Uh oh!

ngoldbaum commented Aug 9, 2024

Uh oh!

colesbury commented Aug 9, 2024

Uh oh!

davidhewitt commented Aug 9, 2024

Uh oh!

ngoldbaum commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colesbury commented Aug 9, 2024

Uh oh!

davidhewitt commented Aug 9, 2024

Uh oh!

davidhewitt commented Aug 9, 2024

Uh oh!

ngoldbaum commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colesbury Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

colesbury Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

colesbury Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngoldbaum commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mejrs commented Aug 14, 2024

Uh oh!

mejrs Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidhewitt Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidhewitt Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

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