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

Initial free threaded bindings #4421

Merged
merged 31 commits into from
Aug 16, 2024
Merged

Conversation

ngoldbaum
Copy link
Contributor

Refs #4265

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

noxfile.py Outdated Show resolved Hide resolved
pyo3-build-config/src/impl_.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/object.rs Show resolved Hide resolved
pyo3-ffi/src/cpython/lock.rs Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
Comment on lines 330 to 332
if build_flags.0.contains(&BuildFlag::Py_GIL_DISABLED) && abi3 {
bail!("Cannot set Py_LIMITED_API and Py_GIL_DISABLED at the same time")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@ngoldbaum ngoldbaum Aug 8, 2024

Choose a reason for hiding this comment

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

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.

pyo3-ffi/src/object.rs Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

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

pyo3-ffi/src/object.rs Show resolved Hide resolved
pyo3-ffi/src/cpython/lock.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/cpython/weakrefobject.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/object.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/object.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/object.rs Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
pyo3-build-config/src/impl_.rs Outdated Show resolved Hide resolved
@mejrs
Copy link
Member

mejrs commented Aug 9, 2024

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
Copy link
Contributor Author

@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
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

ngoldbaum commented Aug 9, 2024

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

@colesbury
Copy link

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
Copy link
Member

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
Copy link
Member

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 👍

@ngoldbaum
Copy link
Contributor Author

ngoldbaum commented Aug 14, 2024

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.

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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) }
    }
}

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

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

ngoldbaum commented Aug 14, 2024

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
Copy link
Contributor Author

ngoldbaum commented Aug 14, 2024

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
Copy link
Member

mejrs commented Aug 14, 2024

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

Comment on lines 9 to 13
impl fmt::Debug for PyMutex {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("PyMutex").finish()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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

pyo3-ffi/src/cpython/lock.rs Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

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

pyo3-ffi/src/cpython/lock.rs Show resolved Hide resolved
Comment on lines +136 to +140
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);
Copy link
Member

Choose a reason for hiding this comment

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

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

pyo3-ffi/src/object.rs Outdated Show resolved Hide resolved
pyo3-build-config/src/impl_.rs Outdated Show resolved Hide resolved
@@ -489,6 +489,35 @@ jobs:
echo PYO3_CONFIG_FILE=$PYO3_CONFIG_FILE >> $GITHUB_ENV
- run: python3 -m nox -s test

test-free-threaded:
Copy link
Member

Choose a reason for hiding this comment

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

Very cool to see this job running, thanks 🙏

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks! 🎉

@davidhewitt 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
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants