-
Notifications
You must be signed in to change notification settings - Fork 758
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
Conversation
pyo3-build-config/src/impl_.rs
Outdated
if build_flags.0.contains(&BuildFlag::Py_GIL_DISABLED) && abi3 { | ||
bail!("Cannot set Py_LIMITED_API and Py_GIL_DISABLED at the same time") | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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 :)
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. |
@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. |
Yeah, I think using atomic integers for the fields is a good idea. I'm not sure if there's much value for exposing For more generic locking, you can use |
Note that using atomic values for the fields probably means we want to consider Alternatively on platforms where |
I think CPython requires atomics support. Are there use-cases for a version of pyo3 on hosts where CPython can't be compiled? |
Are there platforms where |
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 👀 |
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>
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 |
pyo3-ffi/src/object.rs
Outdated
#[cfg(Py_GIL_DISABLED)] | ||
ob_gc_bits: 0, | ||
#[cfg(Py_GIL_DISABLED)] | ||
ob_ref_local: AtomicU32::new(0), |
There was a problem hiding this comment.
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
pyo3-ffi/src/cpython/lock.rs
Outdated
#[repr(C)] | ||
pub struct PyMutex { | ||
_bits: AtomicU8, | ||
} |
There was a problem hiding this comment.
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) }
}
}
pyo3-ffi/src/object.rs
Outdated
#[cfg(Py_GIL_DISABLED)] | ||
_padding: 0, | ||
#[cfg(Py_GIL_DISABLED)] | ||
ob_mutex: unsafe { mem::zeroed::<PyMutex>() }, |
There was a problem hiding this comment.
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?
Hmm, maybe this is what @davidhewitt is referring to above in #4421 (comment) with the bit about |
126c97b
to
4f261c2
Compare
It looks like the lint triggers because of the atomic fields, so declaring the |
Can we not just silence the lint? The copy is intentional. |
4f261c2
to
162a65e
Compare
pyo3-ffi/src/cpython/lock.rs
Outdated
impl fmt::Debug for PyMutex { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
f.debug_struct("PyMutex").finish() | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 🚀
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); |
There was a problem hiding this comment.
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 👍
@@ -489,6 +489,35 @@ jobs: | |||
echo PYO3_CONFIG_FILE=$PYO3_CONFIG_FILE >> $GITHUB_ENV | |||
- run: python3 -m nox -s test | |||
|
|||
test-free-threaded: |
There was a problem hiding this comment.
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 🙏
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
There was a problem hiding this 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! 🎉
Refs #4265
nox -s ffi-check
pass on the free-threaded buildcontinue_on_error
set for the test run.