Conversation
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks for sketching this out! I had a first read, mostly looks like how I expect. I think I spotted an edge case which makes me wonder if this is going to end up a bit hamstrung, need to think about that 🤔
The interaction between AtomicPy / AtomicOptionPy is also a touch unfortunate, but I guess Option<AtomicPy<T>> is also unhelpful for obvious reasons. I guess AtomicPtr is technically nullable, should we just lean into that and only have the AtomicOptionPy form?
src/atomic.rs
Outdated
| // SAFETY: `ptr` is a non-null, borrowed pointer to `ffi::PyObject` of type `T` | ||
| Err(Borrowed::from_ptr_unchecked(py, ptr) | ||
| .to_owned() | ||
| .cast_into_unchecked()) |
There was a problem hiding this comment.
I wonder, is there a potential race here if another thread writes avalue to the AtomicPy dropping this ptr before the .to_owned call?
... is this potentially indicative of a more general problem with AtomicPy, e.g. does load have the same issue? 🤔
There was a problem hiding this comment.
Uff, I think you're right here. I guess this means we can only really provide the swap operation (and probably take for the option variant), because that does not touch the ref-count (at least if we want to keep this a thin wrapper).
I could image providing load and swap if we take more control.
loadwould need to swap with nullptr, increment the ref-count, store back originalloadandswapspin briefly if we're in the middle of aload- we can't expose an
Orderinganymore
Not sure if that would have significant benefits left over a Mutex<Py>.
compare_exchange looks pretty hopeless, I don't see a way to expose that.
There was a problem hiding this comment.
I don't think it's possible to expose this in general, you end up needing something like crossbeam-epoch or arc-swap.
| /// store part of this operation [Relaxed](Ordering::Relaxed), and using | ||
| /// [Release](Ordering::Release) makes the load part [Relaxed](Ordering::Relaxed). | ||
| #[inline] | ||
| pub fn swap<'py>(&self, obj: Bound<'py, T>, order: Ordering) -> Bound<'py, T> { |
There was a problem hiding this comment.
It confused me a bit confusing to have from_bound and from_py on one side and swap_unbound and swap on the other side. What about something like from_bound/from_unbound and swap_bound/swap_unbound?
4 participants
This is an experiment playing with the idea (#5349 (comment)) of an
AtomicPythat can be swapped locked-free using anAtomicPtrinternally. This is basically a really thin layer on top ofAtomicPtr, just to enough to handle the reference counting, so it's still fairly low level. There could also be a companionAtomicOptionPythat makes use of the null-pointer.The primary use case I could imagine for this are
frozenpyclasses (in combination with the free-threaded build). There may be others as well. I would be curious what others think about this.