-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
gh-112606: Use pthread_cond_timedwait_relative_np
in parking_lot.c when available
#112616
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semaphore bits look good. The pthread_cond_timedwait
parts need some work and might be a bit tricky. If you like, you can split it into two PRs, or keep as one if you prefer.
@colesbury Thanks for reviewing. I think I'll go ahead and split the PR here and work on the condvar initialization part. |
pthread_condattr_t
when supported in semaphore waiting in parking_lot.c
pthread_condattr_t
when supported in semaphore waiting in parking_lot.cpthread_cond_timedwait_relative_np
when supported in MacOS semaphore waiting in parking_lot.c
pthread_cond_timedwait_relative_np
when supported in MacOS semaphore waiting in parking_lot.cpthread_cond_timedwait_relative_np
in parking_lot.c when available
Updated |
I would feel more comfortable if Then, if macOS doesn't support pthread_condattr_setclock(), I would also like to see |
@vstinner , likely @colesbury can be more useful about what I am saying, but to me, for the common unix implementation, that should have been dealt with on #112733 which uses |
@vstinner, I agree that it would be better to use the same implementation. In this case, that probably means more use of the |
@mattprodani, can you resolve the conflicts and address the latest round of comments? |
74bd3cd
to
85cbe7d
Compare
Autoconf merge conflict is resolved. It looks like, now, the only case where we could share condition variable implementation between |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configure changes LGTM. I'll leave parking lot for Sam :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM too.
As Victor pointed out, there similar logic in Python/thread_pthread.h
, but I think that refactoring out the common logic is a bit complicated and better left to a subsequent change.
@mattprodani, can you fix the merge conflicts? |
…on in `parking_lot.c` Adds a configure define for `HAVE_PTHREAD_COND_TIMEDWAIT_RELATIVE_NP` and replaces `pthread_cond_timedwait` with `pthread_cond_timedwait_relative_np` for relative time when supported in semaphore waiting logic.
Done, there was a new change to |
Thanks; note that we prefer |
Next, refactoring? :) |
…lot.c when available (python#112616) Add a configure define for HAVE_PTHREAD_COND_TIMEDWAIT_RELATIVE_NP and replaces pthread_cond_timedwait() with pthread_cond_timedwait_relative_np() for relative time when supported in semaphore waiting logic.
Updates parking_lot.c to prefer functions that support CLOCK_MONOTONIC for semaphore and pthread_cond_t based implementations of _PySemaphore_PlatformWait.
Updated: to instead use
pthread_cond_timedwait_relative_np
with a relative timeout which is supported on MacOS, unlikepthread_condattr_setclock(..., CLOCK_MONOTONIC)
. Part of work to makeparking_lot.c
timing more accurate.Linked to PR for POSIX semaphores.