WIP: Release GIL around C functions #391
Conversation
These functions should be safe to use without the Python GIL as they are not creating or destroying Python objects. So mark them as such. Note: This does **not** actually release the GIL. We still have to do that ourselves. However this allows us to use these in code sections where we have already released the GIL. We can also continue to use them when we have the GIL as well.
Cython already infers these are `object` type given the assignment type. So these definitions can be dropped without any loss in affect. This will make it easier to move the assignment into the GIL block later.
As these functions are effectively called from `nogil` contexts now given the C functions calling them are `nogil`, make them `nogil` as well, but acquire the GIL internally for the parts of the function that require it. Namely when the Python callback is run and its results are added to the asyncio Future.
Go ahead and release the GIL before calling UCX functions. They should not be creating, destroying, or modifying any Python objects. Once any of our callbacks are called that do need to manipulate Python objects, we make sure to reacquire the GIL anyways.
As this is the type expected by the C level functions that we call later when using `port`, go ahead and coerce it to `uint16_t` in advance. This should make it easier to release the GIL later on.
jakirkham
force-pushed
the
rel_gil_ucx_c
branch
from
8210540 to
c293849
Compare
| int(<uintptr_t><void*>a.ucp_worker), | ||
| func, | ||
| a.guarantee_msg_order | ||
| with gil: |
There was a problem hiding this comment.
This needs to be with the gil ?
There was a problem hiding this comment.
Oh , any call back needs to be with the gil, correct ?
| _send_cb) | ||
| cdef ucp_send_callback_t _send_cb | ||
| cdef ucs_status_ptr_t status | ||
| with nogil: |
There was a problem hiding this comment.
Should this be with the gil since it's a callback executed by C ?
There was a problem hiding this comment.
Our Cython callbacks are acquiring the GIL before they call into Python. So we need to be sure to release the GIL beforehand.
|
FWIW this still needs a bit more work. There are other places we could potentially benefit from releasing the GIL. |
|
@jakirkham is there a good test for testing GIL release ? |
|
We can call |
|
i didn't know about |
|
I suppose we could add some asserts in the code. Is that what you have in mind? |
|
Yeah, I at not very familiar with adding in GIL release so I am mostly speaking out of ignorance. I checked the Pandas source and I don't think they test GIL release though they do have calls to |
|
There is a merge conflict. Once this is resolved we should be good to merge |
Yeah saw that. Will fix in a bit.
Yeah we should probably hold off merging until we are releasing the GIL in more places. Will take a look at this tomorrow. |
Fixes #251
Mark C functions as
nogil. Adjust the Cython C-level callbacks to not require the GIL (though acquire it internally as needed). Make sure to release the GIL before calling UCX C functions.