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 sem_clockwait with monotonic time when supported in parking_lot.c #112733

Merged
merged 3 commits into from
Dec 6, 2023

Conversation

mattprodani
Copy link
Contributor

@mattprodani mattprodani commented Dec 4, 2023

Completes one part of moving parking_lot.c's semaphore waiting to prefer functions that support CLOCK_MONOTONIC Uses sem_clockwait in the semaphore-based implementation. Split with previous PR to move support for CLOCK_MONOTONIC in pthread_condvar_t implementation to a separate PR.

Completes one part of moving `parking_lot.c`'s semaphore waiting to
prefer functions that support CLOCK_MONOTONIC. Uses sem_clockwait in the
semaphore-based implementation.
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.

Thanks, this looks good to me. @corona10 can you review this please?

@colesbury
Copy link
Contributor

Also cc @vstinner - CLOCK_MONOTONIC fixes in parking_lot.c

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote similar code in Python/thread_pthread.h. Here is my review.


This change only fix one code path. The code path if _Py_USE_SEMAPHORES is not defined still uses the system clock. pthread_condattr_setclock(&ca, CLOCK_MONOTONIC) can be used for that: see Python/thread_pthread.h.


Unrelated to this PR, I have concerns about _PySemaphore_PlatformWait().

(DWORD) (timeout / 1000000) can overflow in the Windows code path. Using _PyTime_As100Nanoseconds() may be a good start.

PEP 475 "Retry system calls failing with EINTR" is not supported: the caller has to handle Py_PARK_INTR. The problem is that it's quite hard to recompute properly a timeout to take in account time elapsed by the call which failed with Py_PARK_INTR. It would be better to retry on EINTR and recompute the timeout. Similar to what Python/thread_pthread.h does in PyThread_acquire_lock_timed(). For the sem_clockwait() code path, it's easy, since we only have to compute an absolute timestamp once.

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

@vstinner Thanks for reviewing, I'm finishing up the remaining pthread_cond_t path similarly to thread_pthread.h but we decided to split it into a different PR. I'll update with the documentation changes.

@corona10 corona10 self-assigned this Dec 6, 2023
@corona10
Copy link
Member

corona10 commented Dec 6, 2023

I will take look this PR today

@corona10 corona10 merged commit a2a46f9 into python:main Dec 6, 2023
32 checks passed
@colesbury
Copy link
Contributor

@vstinner Please see the other PR, which is still a draft, for discussion about the pthreads code path: #112616. I don't think condattr_monotonic helps here because only macOS uses the pthreads code path in parking_lot.c and it doesn't support pthread_condattr_setclock. But we can use the macOS extension pthread_cond_timedwait_relative_np to avoid timezone issues.

(DWORD) (timeout / 1000000) - thanks, filed #112804

"Retry system calls failing with EINTR" - Interrupts are properly retried. It happens at a higher level in the abstraction, see _PyMutex_LockTimed. The timeout computation there copies the pattern from thread_pthread.h.

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