Repository navigation
New GIL: improve condition variable emulation on NT #52658
Description
| BPO | 8411 |
|---|---|
| Nosy | @terryjreedy, @pitrou, @kristjanvalur, @tjguk, @briancurtin |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/pitrou'
closed_at = <Date 2010-08-13.15:13:16.163>
created_at = <Date 2010-04-15.20:35:42.309>
labels = ['interpreter-core', 'performance']
title = 'New GIL: improve condition variable emulation on NT'
updated_at = <Date 2010-08-13.15:13:16.162>
user = 'https://github.com/kristjanvalur'bugs.python.org fields:
activity = <Date 2010-08-13.15:13:16.162>
actor = 'pitrou'
assignee = 'pitrou'
closed = True
closed_date = <Date 2010-08-13.15:13:16.163>
closer = 'pitrou'
components = ['Interpreter Core']
creation = <Date 2010-04-15.20:35:42.309>
creator = 'kristjan.jonsson'
dependencies = []
files = ['16934', '18442']
hgrepos = []
issue_num = 8411
keywords = ['patch', 'needs review']
message_count = 12.0
messages = ['103251', '113029', '113089', '113301', '113303', '113314', '113315', '113318', '113400', '113540', '113559', '113783']
nosy_count = 6.0
nosy_names = ['terry.reedy', 'pitrou', 'kristjan.jonsson', 'jyasskin', 'tim.golden', 'brian.curtin']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue8411'
versions = ['Python 3.2']
Activity
kristjanvalur commentedon Apr 15, 2010
As per Antoine's suggestion, here is a patch to improve condition variable emulation on windows.
By using the windows Semaphore (which hasn't always been available) all of the problems in emulating condition variables using Events disappear. in particular, the "lost wakeup" bug can be elimintated, and the same object easily supports bot the "signal" and "broadcast" mode of waking up threads. We can also use the CRICITAL_SECTION objects as the associated mutex which is much lighter weight than using a kernel object.
I do think I´ve caught all the corner cases. It is also simpler to prove because this construct is simpler than the previous one. If, however, i have missed a spot, I'd be interested to know about it. Not completely impossible since race conditions are devious beasts. Ive tested this extensively on Windows.
See also bpo-8299 where this implementation was presented.
terryjreedy commentedon Aug 5, 2010
This patches ceval_gil.h. Can these changes be unit tested?
Or is this intended to be an internal performance, invisible-to-the-user, code improvement?
Antoine, are you going to look at this?
kristjanvalur commentedon Aug 6, 2010
The latter. But I'd really like Antoine to look this over.
pitrou commentedon Aug 8, 2010
Questions:
why does _cond_timed_wait() decrement n_waiting *twice*?
why does _cond_timed_wait() use InterlockedDecrement()? it doesn't protect against regular accesses to the same memory location, since it isn't guarded by the mutex
why don't you simply pass NULL as the third parameter to ReleaseSemaphore() in _cond_signal()?
I don't understand what you call "possible race condition" in _cond_signal(). _cond_signal() is currently always called with the corresponding mutex held, by the way. We could add a comment to make it mandatory.
pitrou commentedon Aug 8, 2010
Oh, and I would call CreateSemaphore() with a max count of 1, precisely to guard us against bugs.
kristjanvalur commentedon Aug 8, 2010
Thanks for your comments.
The problem with _cond_timed_wait indicates that the timed wait code hadn't been thoroughly tested at the time of submission.
As for the race condition, it is as follows:
a) Thread A is waiting in _cond_timed_wait(), n_waiting == 1
b) The wait times out and it tries to acquire the lock to decrement n_waiting. This is a critical lock, because no one is waiting for the semaphore, yet n_waiting == 1
c) Thread B, that has the lock, calls _cond_signal(). It still sees n_waiting == 1 and does a ReleaseSemaphore. It then also decrements n_waiting, and n_waiting is now 0. The Semaphore count is now 1
d) Thread A finally gets the lock and decrements n_waiting. n_waiting is now -1.
n_waiting has thus been decremented twice. But this is ok, since there was also an extra ReleaseSemaphore call and the semaphore count is now 1. The upshot of this is that this race condition is benign, and what happens is that the next time someone does _cond_wait(), the semaphore is reduced to 0, n_waiting goes to 0, and the thread passes right through. Thus, should this event happen, the only upshot of it is that one _cond_wait call will be ineffective.
I'll upload a new patch with better explanation.
kristjanvalur commentedon Aug 8, 2010
"this is a critical lock" should read "this is a critical moment" :)
kristjanvalur commentedon Aug 8, 2010
Updated patch file
kristjanvalur commentedon Aug 9, 2010
The semaphore value going above 0 is not a "bug" but an implementation detail stemming from the fact that n_waiting cannot reliably reflect the semaphore state at each particular time. Due to locking, it may lag behind the state a little bit.
pitrou commentedon Aug 10, 2010
Thanks Kristján, I've committed the patch with modified comments in r83932.
Unfortunately, the Windows buildbots are in a wreck, so this won't be checked immediately (I only have a single-core XP VM under which the patch ran fine).
[-]Improve condition variable emulation on NT[/-][+]New GIL: improve condition variable emulation on NT[/+]kristjanvalur commentedon Aug 10, 2010
Great. I hope you agree that it is simpler. I'm afraid my explanations can be somewhat long-winded so I hope you found a better way to document my "pseudo race condition".
pitrou commentedon Aug 13, 2010
Closing as fixed, since nothing strange seems to have appeared so far.