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

Potential enhancement: GIL-based Mutex #1877

Closed
itamarst opened this issue Sep 18, 2021 · 8 comments
Closed

Potential enhancement: GIL-based Mutex #1877

itamarst opened this issue Sep 18, 2021 · 8 comments

Comments

@itamarst
Copy link
Contributor

I have found myself creating statics that are accessed only by threads that have the GIL. As such, use of a normal Mutex is unnecessary performance overhead (and this matters somewhat in my use case, a profiler).

There is Py<T>, but that assumes T is a Python object, so random Rust objects are harder.

So, here is a sketch of a GIL-based Mutex:

/// Protect access to an object such that it can only be accessed when the GIL
/// is held.
pub struct GilMutex<T: ?Sized> {
    data: std::cell::RefCell<T>,
}

unsafe impl<T: ?Sized + Send> Sync for GilMutex<T> {}

pub struct GilRefMut<'p, 's, T: ?Sized> {
    inner: std::cell::RefMut<'s, T>,
    _py: Python<'p>,
}

impl<T: ?Sized> Deref for GilRefMut<'_, '_, T> {
    type Target = T;

    #[inline]
    fn deref(&self) -> &T {
        &self.inner
    }
}

impl<T: ?Sized> DerefMut for GilRefMut<'_, '_, T> {
    #[inline]
    fn deref_mut(&mut self) -> &mut T {
        self.inner.borrow_mut()
    }
}

impl<T> GilMutex<T> {
    /// Create a new mutex.
    pub const fn new(t: T) -> Self {
        GilMutex {
            data: std::cell::RefCell::new(t),
        }
    }

    /// Get a mutable reference to the data, protected by the GIL.
    pub fn lock<'p, 's>(&'s self, py: Python<'p>) -> GilRefMut<'p, 's, T> {
        GilRefMut {
            inner: self.data.borrow_mut(),
            _py: py,
        }
    }
}
  1. Does this sound generally useful enough to add to PyO3?
  2. Is this code sound? I don't know enough Rust to be certain.
@mejrs
Copy link
Member

mejrs commented Sep 19, 2021

Acquiring an uncontested mutex is really cheap. I think you're overestimating the overhead associated with that. Especially the difference between it and a RefCell, and the relative costs when things like once_cell/lazy_static get involved.

It's shame we can't guarantee unique access with the Python token (it's Copy), this would have been a cool application for it.

@itamarst
Copy link
Contributor Author

Ah, hadn't thought about the uncontested part. I don't need once_cell/lazy_static, const initialization is sufficient in my case. As I said, I really care about performance for my particular application (it's a memory profiler, I'm doing things like running code on every Python frame start/end and every malloc()/free()), and even small amounts of work add up. But that's probably not representative of most use cases, yes.

So I guess next step would be a benchmark to see if it makes a difference.

@davidhewitt
Copy link
Member

This is actually very similar to the PyCell concept we already have, except that PyCell is heavily coupled to the layout of #[pyclass] types. I would like to eventually decouple the two, so that PyCell becomes something which could optionally be used to wrap the whole struct. (#1068)

At that point I think it definitely makes sense to make it a reusable API which users could wrap around individual fields; that would enable a kind of interior mutability for #[pyclass] objects. And as you originally propose here, that would also have use in statics.

So in summary, yes, I would argue there is space for this concept in PyO3. However, I would prefer not introduce it until after we have implemented #1068, because at the moment I think it conflicts a bit with the existing PyCell concept.

(I also think that given this is a wrapper around RefCell, either PyCell or GilCell seems like a more correct name than calling it a mutex.)

@davidhewitt
Copy link
Member

Now that we've added #[pyclass(immutable)] I think that this proposal is very relevant for consideration for 0.17.

@davidhewitt davidhewitt added this to the 0.17 milestone Apr 26, 2022
@davidhewitt
Copy link
Member

A passing thought: how is this impacted by the possibility of multiple sub-interpreters (and eventually multiple GILs - see PEP 684)?

@davidhewitt davidhewitt modified the milestones: 0.17, 0.18 Jul 3, 2022
@davidhewitt davidhewitt modified the milestones: 0.18, 0.19 Dec 27, 2022
@lifthrasiir
Copy link
Contributor

Isn't this eventually fixed by #2975 (GILProtected)?

@adamreichold
Copy link
Member

Yes, I think GILProtected does provide what is asked here - protecting data based on having acquired the GIL. It is not really a mutex as it does not handle re-entrancy on a single thread, so it is maybe a bit closer to thread_local!. But in both cases, mutable access can easily be achieved by adding a RefCell layer as we also highlight in the example for GILProtected.

@adamreichold adamreichold modified the milestones: 0.19, 0.18 May 29, 2023
@adamreichold
Copy link
Member

Change the milestone as we released this as part of 0.18.3. (The impact of PEP 684 is also not clear but that affects all GIL-based synchronization in PyO3, e.g. at least PyCell, GILOnceCell and GILProtected.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants