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

WIP: Mark C level functions as nogil #383

Closed
wants to merge 4 commits into from

Conversation

jakirkham
Copy link
Member

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.

@jakirkham jakirkham requested a review from a team as a code owner January 14, 2020 04:15
@jakirkham jakirkham force-pushed the use_nogil_extern branch 2 times, most recently from 2c7d772 to 5390564 Compare January 14, 2020 05:59
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.
@quasiben
Copy link
Member

Thansk @jakirkham . Looks like CI is failing with legitimate errors:

ucp/_libs/send_recv.pyx:90:51: Cannot assign type 'void (void *, ucs_status_t)' to 'ucp_send_callback_t'

@jakirkham
Copy link
Member Author

Yeah that should be fixed by PR ( #390 ).

@jakirkham
Copy link
Member Author

Even though this passes. We should hold off on merging for a bit as there are some warning. These highlight some additional changes we need.

From the log we can see the following warnings:

warning: ucp/_libs/send_recv.pyx:85:40: Casting a GIL-requiring function into a nogil function circumvents GIL validation
warning: ucp/_libs/send_recv.pyx:132:8: Casting a GIL-requiring function into a nogil function circumvents GIL validation
warning: ucp/_libs/send_recv.pyx:148:40: Casting a GIL-requiring function into a nogil function circumvents GIL validation
warning: ucp/_libs/send_recv.pyx:195:8: Casting a GIL-requiring function into a nogil function circumvents GIL validation
warning: ucp/_libs/core.pyx:353:12: Casting a GIL-requiring function into a nogil function circumvents GIL validation
warning: ucp/_libs/core.pyx:424:12: Casting a GIL-requiring function into a nogil function circumvents GIL validation

This is happening because the C functions potentially may not hold the GIL when calling our Cython callbacks. We can fix this by defining our Cython callbacks to also not hold the GIL, but acquire it internally when it is needed.

Will make some changes to this affect in a second.

@jakirkham jakirkham changed the title Mark C level functions as nogil WIP: Mark C level functions as nogil Jan 14, 2020
@jakirkham
Copy link
Member Author

To fix the warnings, it makes to type our callbacks as nogil. However as we still need the gil within the callbacks, we need to acquire the GIL internally. This means we should also release the GIL before calling into C functions. Given all of this, it makes more sense to start a new PR that covers these as well. Have added PR ( #391 ).

@jakirkham jakirkham closed this Jan 14, 2020
@jakirkham jakirkham deleted the use_nogil_extern branch January 14, 2020 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants