-
Notifications
You must be signed in to change notification settings - Fork 759
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
Conversation
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, good to see that this is a relatively straightforward change in PyO3 at least!
}; | ||
use std::cell::UnsafeCell; | ||
|
||
#[cfg(not(Py_GIL_DISABLED))] | ||
use crate::PyVisit; | ||
|
||
/// Value with concurrent access protected by the GIL. |
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 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).
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’ll take a look at adding and documenting safe wrappers for PyMutex
that pyo3 can expose in a followup.
44c040a
to
e33212c
Compare
|
||
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. |
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 the end is GILProtected
the only feature we're hiding? It looks like #4512 leaves GILOnceCell
exposed.
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.
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.
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!).
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.
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.
fe0b5cd
to
f1bf682
Compare
f1bf682
to
c1db75d
Compare
}); | ||
# } | ||
``` | ||
|
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'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.
632ff75
to
af80134
Compare
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 looks good to me, thanks. It strikes me that the semantics of LazyTypeObject
still need a fair bit of understanding as we change GILOnceCell
, hopefully I can make further progress on those PRs shortly...
|
||
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. |
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.
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.
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 looks good to me, thanks. It strikes me that the semantics of LazyTypeObject
still need a fair bit of understanding as we change GILOnceCell
, hopefully I can make further progress on those PRs shortly...
Ref #4265 (comment)
This removes
GILProtected
whenPy_GIL_DISABLED
is set.I replaced the use of
GILProtected
inLazyTypeObject
with astd::sync::Mutex
. I didn't hit any deadlocks with the new lock in my testing.Also added a new migration guide section for free-threaded python support and a subsection on
GILProtected
.