gh-112606: Use pthread_cond_timedwait_relative_np in parking_lot.c when available#112616
gh-112606: Use pthread_cond_timedwait_relative_np in parking_lot.c when available#112616erlend-aasland merged 2 commits intopython:mainfrom
pthread_cond_timedwait_relative_np in parking_lot.c when available#112616Conversation
colesbury
left a comment
There was a problem hiding this comment.
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 |
erlend-aasland
left a comment
There was a problem hiding this comment.
Configure changes LGTM. I'll leave parking lot for Sam :)
colesbury
left a comment
There was a problem hiding this comment.
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_npwith a relative timeout which is supported on MacOS, unlikepthread_condattr_setclock(..., CLOCK_MONOTONIC). Part of work to makeparking_lot.ctiming more accurate.Linked to PR for POSIX semaphores.