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

gh-112606: Use pthread_cond_timedwait_relative_np in parking_lot.c when available #112616

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

mattprodani
Copy link
Contributor

@mattprodani mattprodani commented Dec 2, 2023

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, unlike pthread_condattr_setclock(..., CLOCK_MONOTONIC). Part of work to make parking_lot.c timing more accurate.

Linked to PR for POSIX semaphores.

Copy link
Contributor

@colesbury colesbury left a 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.

Python/parking_lot.c Outdated Show resolved Hide resolved
Python/parking_lot.c Outdated Show resolved Hide resolved
@mattprodani
Copy link
Contributor Author

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.

@mattprodani mattprodani changed the title gh-112606: Use monotonic clock when supported in semaphore waiting in parking_lot.c gh-112606: Use monotonic clock pthread_condattr_t when supported in semaphore waiting in parking_lot.c Dec 4, 2023
@mattprodani mattprodani changed the title gh-112606: Use monotonic clock pthread_condattr_t when supported in semaphore waiting in parking_lot.c gh-112606: Use pthread_cond_timedwait_relative_np when supported in MacOS semaphore waiting in parking_lot.c Dec 6, 2023
@mattprodani mattprodani marked this pull request as ready for review December 6, 2023 20:44
Python/parking_lot.c Outdated Show resolved Hide resolved
@colesbury colesbury changed the title gh-112606: Use pthread_cond_timedwait_relative_np when supported in MacOS semaphore waiting in parking_lot.c gh-112606: Use pthread_cond_timedwait_relative_np in parking_lot.c when available Dec 6, 2023
@mattprodani
Copy link
Contributor Author

Updated parking lot.c PR: @vstinner @colesbury - use macOS relative clock specific pthread_cond_timedwait_relative_np as discussed.

@vstinner
Copy link
Member

vstinner commented Dec 7, 2023

I would feel more comfortable if Python/thread_pthread.h and Python/parking_lot.c would use the same implementation (more or less). So I would suggest to start first with pthread_condattr_setclock() to support most Unix.

Then, if macOS doesn't support pthread_condattr_setclock(), I would also like to see Python/thread_pthread.h being modified to use it.

@mattprodani
Copy link
Contributor Author

@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 sem_clockwait on plain semaphores as a base rather than a pthread condvar/mutex pair. (as available). The remaining branch that uses a condition variable and a mutex seems to mainly target macOS, which does not support pthread_condattr_setclock or sem_clockwait.

@colesbury
Copy link
Contributor

@vstinner, pthread_condattr_setclock would just be dead code in parking_lot.c. All the other Unixes use the semaphore code path (Linux, FreeBSD, AIX, etc.). Additionally, it's a bit tricky to do the attribute initialization once in a thread-safe manner.

I agree that it would be better to use the same implementation. In this case, that probably means more use of the condvar.h file. (thread_pthread.h is not suitable for re-use -- it can only be included once in thread.c). That still requires a bit of a refactor due to the use of microseconds vs. nanoseconds, so I'd prefer to fix the immediate issue and then follow up with the refactoring.

@erlend-aasland
Copy link
Contributor

@mattprodani, can you resolve the conflicts and address the latest round of comments?

@mattprodani
Copy link
Contributor Author

@mattprodani, can you resolve the conflicts and address the latest round of comments?

Autoconf merge conflict is resolved.

It looks like, now, the only case where we could share condition variable implementation between thread_pthread.h and parking_lot.c is on Mac. Unless we want to work on refactoring condvar.h now, the PR should be complete as it fixes the immediate issue as discussed by @colesbury's last comment.

Copy link
Contributor

@erlend-aasland erlend-aasland left a 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 :)

Copy link
Contributor

@colesbury colesbury left a 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.

@erlend-aasland
Copy link
Contributor

@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.
@mattprodani
Copy link
Contributor Author

mattprodani commented Jan 29, 2024

@mattprodani, can you fix the merge conflicts?

Done, there was a new change to configure. Decided to rebase here but parking_lot.c remains unchanged from last review.

@erlend-aasland
Copy link
Contributor

Done, there was a new change to configure. Decided to rebase here but parking_lot.c remains unchanged from last review.

Thanks; note that we prefer git merge --no-ff main to rebase and force-push; it plays better with the GitHub UI. See also the devguide.

@erlend-aasland erlend-aasland merged commit e5e1866 into python:main Jan 30, 2024
30 checks passed
@erlend-aasland
Copy link
Contributor

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.

Next, refactoring? :)

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants