Enable GILProtected access via PyVisit

[neachdainn]

Closes #3615

This simply adds a new method which uses the existence of a PyVisit object as proof that the GIL is held instead of a Python object. This allows GILProtected to be used in instances where contained Python objects need to participate in garbage collection. Usage in this situation should be valid since no Python calls are made and this does not provide any additional mechanism for accessing a Python object.

[neachdainn]

I am 100% willing to change the name of the method. I just wanted the MR to go with the issue ASAP so both can be closed quickly (assuming there is little to no need to discuss the feature).

[davidhewitt]

I'm feeling split about this feature. I understand why in practice this seems ok at the moment, but can we really rely on the fact that the GC will never traverse the graph in parallel?

Can you give some use cases of what you need this for? With the advent of nogil, we sort of need to be killing GILProtected so perhaps we can work out alternative patterns...

[neachdainn]

can we really rely on the fact that the GC will never traverse the graph in parallel?

If we can't, then PyO3 already has a major issue around marking types as unsendable. That being said, I think we can based on the fact that making the garbage collector parallel would be a major happening in Python and would probably break a fair number of existing extensions.

EDIT: Ironically, reading PEP 703, it sounds like even with nogil all other Python threads will be paused during cycle detection.

Can you give some use cases of what you need this for?

Any time you have any Python object behind a GILProtected, you would need this in order to enroll in GC correctly. My specific use case is I am implementing something similar to a MPMC channel and I want to leverage the fact that the GIL is present in order to not have to pull in another mutex or deal with atomics. The vastly oversimplified version looks something like this:

struct Inner {
    items: VecDeque<PyObject>,
}

#[pyclass]
struct Sender {
    inner: Arc<GILProtected<RefCell<Inner>>>,
}

#[pymethods]
impl Sender {
    fn send(&self, py: Python<'_>, obj: PyObject) {
        self.inner.get(py).borrow_mut().items.push_back(obj);
    }
    
    fn __traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError> {
        // unable to traverse objects in the queue
    }
    
    fn __clear__(&mut self, py: Python<'_>) {
        self.inner.get(py).borrow_mut().items.clear();
    }
}

With the advent of nogil, we sort of need to be killing GILProtected so perhaps we can work out alternative patterns...

I'll need to read up on the nogil but I'm not opposed to alternative patterns. I mostly want these changes so I can remove instances of self.inner.get(Python::assume_gil_acquired()) from my code. Didn't click for a second that you were talking about PEP 703 - I thought you were talking about the Py2<'py, T> stuff.

I don't necessarily see that as an issue in this MR. When PEP 703 does land, my money is on it taking a while for it to actually be used and, more importantly, that's a massive, fundamental, update to PyO3. It doesn't seem to me that a three line convenience function should be blocked by something that big and far away.

[davidhewitt]

Fair points! Good thought about unsendable too, I vaguely recall hearing that the GC runs in a dedicated thread on nogil so we might indeed have a problem there, but that's a different discussion.

I think for practicality's sake I'm happy to move forward here with the name traverse, and when we start dealing with nogil (hopefully on my target for early in the new year) we can figure out the replacements. I suspect that GILProtected<RefCell<T>> will be best served by something like RwLock<T>.

[adamreichold]

Please also fix the reference to the original method name in the changelog entry. Thanks!

[neachdainn]

Please also fix the reference to the original method name in the changelog entry. Thanks!

Good catch. Done!

[codspeed-hq]

CodSpeed Performance Report

Merging #3616 will improve performances by 22.03%

_{Comparing neachdainn:3615-gilprotected-pyvisit (3249feb) with main (856c573)}

Summary

⚡ 1 improvements
✅ 77 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	neachdainn:3615-gilprotected-pyvisit	Change
⚡	not_a_list_via_downcast	153.9 ns	126.1 ns	+22.03%