-
Notifications
You must be signed in to change notification settings - Fork 60
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
Releasing the GIL #251
Comments
@madsbk, do you have any thoughts on this? |
I think we should be able to release the GIL at most UCX API calls but they are all non-blocking so I don't think it will have a significant impact on the performance. |
Agreed. I would wait on this until profiling showed that it was important.
…On Thu, Oct 17, 2019 at 3:33 PM Mads R. B. Kristensen < ***@***.***> wrote:
I think we should be able to release the GIL at most UCX API calls but
they are all non-blocking so I don't think it will have a significant
impact on the performance.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#251?email_source=notifications&email_token=AACKZTG37ZLNLHVUSE4WAL3QPDD2RA5CNFSM4JAUVFTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBRN3QA#issuecomment-543350208>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKZTAU2LV74MEDMLFKJ63QPDD2RANCNFSM4JAUVFTA>
.
|
I'm not as concerned about performance speedup as handling major issues like hangs at this stage. The latter hurts usability so is worth thinking about IMHO. |
I agree that hangs are bad. Are you saying that holding onto the GIL is
causing UCX to hang?
…On Thu, Oct 17, 2019 at 3:52 PM jakirkham ***@***.***> wrote:
I'm not as concerned about performance speedup as handling major issues
like hangs at this stage. The latter hurts usability so is worth thinking
about IMHO.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#251?email_source=notifications&email_token=AACKZTFNHF5B3WYQK3BML4TQPDGCBA5CNFSM4JAUVFTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBRPQ5Y#issuecomment-543357047>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKZTBAKWDKODNPXQG7SIDQPDGCBANCNFSM4JAUVFTA>
.
|
Wanted to check and see if there would be any problem pursuing this path should it be needed. Am hearing there shouldn’t be which is good. |
In my mind it might be helpful, but really we should focus on getting a
benchmark up and running quickly so that we can make these kinds of
determinations. I think that once someone is actually playing with
dask-cudf joins then we'll know a ton more about what work needs to be
done. I'm personally less excited about cleanup work until we have a
benchmark to help give direction and priorities.
…On Thu, Oct 17, 2019 at 4:40 PM jakirkham ***@***.***> wrote:
Wanted to check and see if there would be any problem pursuing this path
should it be needed. Am hearing there shouldn’t be which is good.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#251?email_source=notifications&email_token=AACKZTFXDRBUVXPFAKMGLUTQPDLUJA5CNFSM4JAUVFTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBRTOHQ#issuecomment-543373086>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKZTENFX2RKPPUKIGZEJ3QPDLUJANCNFSM4JAUVFTA>
.
|
Currently we don't release the GIL in the code. Perhaps this is something we should? If so, it would be good to identify where we can release the GIL. If this is not something we should do, it would be good to understand why.
The text was updated successfully, but these errors were encountered: