Thread-safe release pools #2
Conversation
|
Thank you! |
| // Note: inside this closure we must be careful to not hold a borrow too long, because | ||
| // while calling Py_DECREF we may cause other callbacks to run which will need to | ||
| // register objects into the GILPool. | ||
| let len = owned_objects.borrow().len(); |
There was a problem hiding this comment.
Hmm 🤔
If we need this kind of hack, I think we should consider simply locking other threads.
There was a problem hiding this comment.
The problem isn't other threads, but that multiple GILPools can exist in the same thread. They might even be created & dropped while this GILPool's drop is running.
For example, when Py_DECREF causes a reference count to go to zero, then we may end up running a tp_dealloc callback, which might create a new GILPool, which might run other python code, which might run something else which creates another GILPool, ... (and so on). I think that all those GILPools should be cleaned up by the time the tp_dealloc callback ends, but I was feeling cautious.
If we are concerned about performance we should write some more benchmarks to compare PyO3#887 against pyo3 master? If there's a noticeable performance regression then we could consider using UnsafeCell instead of RefCell. I'd prefer avoid the unsafe unless we really need it though.
There was a problem hiding this comment.
Another alternative is maybe for each GILPool to own its own Vec storage. We could even try using smallvec which might mean that most GILPools will never need an allocation?
There was a problem hiding this comment.
EDIT: thinking about this more, we already use UnsafeCell in this file. So, maybe we should just proceed and use UnsafeCell here too.
There was a problem hiding this comment.
(Do you want me to open another PR to change RefCell -> UnsafeCell?)
There was a problem hiding this comment.
Haha, one last thought, sorry :D
Actually I'm not sure that UnsafeCell is sound. Any register_owned call while the other callbacks are running might cause the Vec to resize, which could move the contents of the Vec storage. So we have to be very careful to make sure we're using the shared Vec correctly, else we're accessing invalid memory.
This problem also exists without thread_local, I guess. Just now that we're looking into this file we see the dangers of the existing implementation!
There was a problem hiding this comment.
If we are concerned about performance
I don't mind performance so much here, but now I don't feel this solution so reliable.
There was a problem hiding this comment.
My temporal solution: PyO3@75c807f
I managed to get
thread_local!working usingRefCell. If we wanted to be ambitious we could useUnsafeCellfor performance, but I wanted to be conservative (I think I'd rather know if we got this wrong and take the light performance penalty for now?).At the same time I also was able to clean up
ReleasePoola lot, to not needReleasePoolImpl.I believe this fixes PyO3#756