[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Possibly the patch had a mixup I'm going to rework it a bit and post as a separate issue. -- status: open -> closed ___ Python tracker

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: I agree that Martin that it's not a good idea to add "dead" code. Furthermore, you patch has: +#ifndef _PY_EMULATED_WIN_CV +#define _PY_EMULATED_WIN_CV 0 /* use emulated condition variables */ +#endif + +#if !defined NTDDI_VISTA || NTDDI_VERSION < NTDDI_VISTA

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Again, to clarify because this seems to have been put to sleep by Martin's unfortunate dismissal. A recap of the patch: 1) Extract the Contition Variable functions on windows out of ceval_gil.h and into thread_nt_cv.h, so that they can be used in mo

[issue11618] Locks broken wrt timeouts on Windows

nal Message- > From: Martin v. Löwis [mailto:rep...@bugs.python.org] > Sent: 30. apríl 2012 09:05 > To: Kristján Valur Jónsson > Subject: [issue11618] Locks broken wrt timeouts on Windows > > > Martin v. Löwis added the comment: > > As it stands, the patch is pointless,

[issue11618] Locks broken wrt timeouts on Windows

Martin v. Löwis added the comment: As it stands, the patch is pointless, and can safely be rejected. We will just not have defined NTDDI_VERSION at NTDDI_VISTA for any foreseeable future, so all the Vista-specific code can be eliminated from the patch. Python had been using dynamic checking f

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Ok, but the patch as provided would become more compliated. For general consumption, the primitives would need to become dynamically allocated structures, and so on. I'm not sure that its worth the effort, but I can have a look. (I thought the patc

[issue11618] Locks broken wrt timeouts on Windows

Brian Curtin added the comment: We do the runtime checks for a few things in winreg as well as the os.symlink implementation and i think a few other supplemental functions for symlinking. -- ___ Python tracker __

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: I understand what he meant, but that wasn't the intent of the patch. The patch is to use simulated critical sections using a semaphore, same as the new GIL implementation already does. If you want dynamic runtime detection, then this is a feature req

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > This is an XP patch. The "vista" option is put in there as a compile > time option, and disabled by hand. I'm not adding any apis that > weren't already in use since the new gil (windows Semaphores). Martin means that you shouldn't use #ifdef's but runtime

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Antoine: of course, sorry for rushing you. Martin, This is an XP patch. The "vista" option is put in there as a compile time option, and disabled by hand. I'm not adding any apis that weren't already in use since the new gil (windows Semaphores).

[issue11618] Locks broken wrt timeouts on Windows

Martin v. Löwis added the comment: -1. Choice of operating system must be a run-time decision, not a compile-time decision. We will have to support XP for quite some time. -- ___ Python tracker __

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > So, what do you think, should this go in? Any qualms about the > thread_nt_cv.h header? On the principle it's ok, but I'd like to do a review before it goes in :) -- ___ Python tracker

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: So, what do you think, should this go in? Any qualms about the thread_nt_cv.h header? -- ___ Python tracker ___ _

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Here is a new patch. I've factored out the NT condittion variable code into thread_nt_cv.h which is now used by both thread_nt.h and ceval_gil.h -- Added file: http://bugs.python.org/file25351/ntlocks.patch

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: The vista specific path is included there for completeness, if and when code moves to that platform, besides showing what the "emulated CV" is actually emulating. Also, I am aware of the old/new GIL, but our console game uses python 2.7 and the old G

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > Is a 60% performance increase for the common case of acquiring an > uncontested lock worth doing? Yes, I agree it is. However, the Vista-specific path seems uninteresting, if it's really 2% faster. > our console profilers were alarmed at all the kernel tran

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Any thougts? Is a 60% performance increase for the common case of acquiring an uncontested lock worth doing? Btw, for our console game I also opted for non-semaphore based locks in thread_pthread.h, because our console profilers were alarmed at all t

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Two runs with standard locks: D:\pydev\hg\cpython2>pcbuild\amd64\python.exe -m timeit -s "import _thread; l = _thread.allocate_lock()" l.acquire();l.release() 100 loops, best of 3: 0.746 usec per loop D:\pydev\hg\cpython2>pcbuild\amd64\python.exe

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > This uses critical sections and condition variables to avoid kernel > mode switches for locks. Windows mutexes are expensive and for > uncontented locks, this offers a big win. Can you post some numbers? -- ___ Py

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Here is a new patch. This uses critical sections and condition variables to avoid kernel mode switches for locks. Windows mutexes are expensive and for uncontented locks, this offers a big win. It also adds an internal set of critical section/conditio

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: I have now committed the semaphore implementation, so as to fix the issue. Potential performance optimizations can still be discussed, of course (either here or in a new issue, I'm not sure). -- resolution: -> fixed stage: -> committed/rejected statu

[issue11618] Locks broken wrt timeouts on Windows

Roundup Robot added the comment: New changeset 9b12af6e9ea9 by Antoine Pitrou in branch '3.2': Issue #11618: Fix the timeout logic in threading.Lock.acquire() under http://hg.python.org/cpython/rev/9b12af6e9ea9 New changeset 9d658f000419 by Antoine Pitrou in branch 'default': Issue #11618: Fix

[issue11618] Locks broken wrt timeouts on Windows

sbt added the comment: krisvale wrote: So, I suggest a change in the comments: Do not claim that the value is never an underestimate, and explain how falsely returning a WAIT_TIMEOUT is safe and only occurs when the lock is heavily contented. Sorry for being so nitpicky but having this

[issue11618] Locks broken wrt timeouts on Windows

Changes by sbt : Removed file: http://bugs.python.org/file21335/locktimeout3.patch ___ Python tracker ___ ___ Python-bugs-list mailing list Un

[issue11618] Locks broken wrt timeouts on Windows

sbt added the comment: krisvale wrote: So, I suggest a change in the comments: Do not claim that the value is never an underestimate, and explain how falsely returning a WAIT_TIMEOUT is safe and only occurs when the lock is heavily contented. Sorry for being so nitpicky but having this

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Sbt: I re-read the code and while I still maintain that the evaluation in line 50 is meaningless, I agree that the worst that can happen is an incorrect timeout. It is probably harmless because this state is only encountered for timeout==0, and it is

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Martin: I wouldn't worry too much about replacing a "Mutex" with a "Semaphore". There is no reason to believe that they behave in any way different scheduling wise, and if they did, then any python code that this would affect would be extremely poorl

[issue11618] Locks broken wrt timeouts on Windows

Martin v. Löwis added the comment: > I realize that this is > a subtle point, but in fact, the atomic functions make no memory > barrier guarantees either (I think). No need to guess: http://msdn.microsoft.com/en-us/library/ms683560(v=vs.85).aspx "This function generates a full memory barrier

[issue11618] Locks broken wrt timeouts on Windows

Martin v. Löwis added the comment: > I would still favour committing the semaphore-based version first > (especially in 3.2), and then discussing performance improvements if > desired. For 3.2, I would prefer a solution that makes least changes to the current code. This is better than fundament

[issue11618] Locks broken wrt timeouts on Windows

sbt added the comment: krisvale wrote There is no barrier in use on the read part. I realize that this is a subtle point, but in fact, the atomic functions make no memory barrier guarantees either (I think). And even if they did, you are not using a memory barrier when you read the 'ti

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > Antoine: I notice that even the fast path contains a ResetEvent() > call. I think this is a kernel call and so just as expensive as > directly using a semaphore :) Yes, in my timings it doesn't show significant improvements compared to the semaphore approac

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Antoine: I notice that even the fast path contains a ResetEvent() call. I think this is a kernel call and so just as expensive as directly using a semaphore :). Otherwise, the logic looks robust, although ResetEvent() and Event objects always give

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: There is no barrier in use on the read part. I realize that this is a subtle point, but in fact, the atomic functions make no memory barrier guarantees either (I think). And even if they did, you are not using a memory barrier when you read the 'tim

[issue11618] Locks broken wrt timeouts on Windows

sbt added the comment: sbt wrote: - I see your point. Still, I think we still may have a flaw: The statement that (owned-timeouts) is never an under-estimate isn't true on modern architectures, I think. The order of the atomic decrement operations in the code means nothing and cannot b

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Antoine: I agree, the semaphore is the quick and robust solution. sbt: I see your point. Still, I think we still may have a flaw: The statement that (owned-timeouts) is never an under-estimate isn't true on modern architectures, I think. The order

[issue11618] Locks broken wrt timeouts on Windows

sbt added the comment: > Btw, the locktimeout.patch appears to have a race condition. > LeaveNonRecursiveMutex may SetEvent when there is no thread waiting > (because a timeout just occurred, but the thread on which it happened > is still somewhere around line #62 ). This will cause the nex

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: Just for the record, here is the critical section-based version. I would still favour committing the semaphore-based version first (especially in 3.2), and then discussing performance improvements if desired. -- Added file: http://bugs.python.org/file

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Btw, the locktimeout.patch appears to have a race condition. LeaveNonRecursiveMutex may SetEvent when there is no thread waiting (because a timeout just occurred, but the thread on which it happened is still somewhere around line #62 ). This will ca

[issue11618] Locks broken wrt timeouts on Windows

sbt added the comment: Benchmarks (on an old laptop running XP without a VM) doing D:\Repos\cpython\PCbuild>python -m timeit -s "from threading import Lock; l = Lock()" "l.acquire(); l.release()" 100 loops, best of 3: 0.934 usec per loop default:0.934 locktimeout.patch: 0.965

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Emulating condition variables on windows became easy once Semaphores were provided by the OS because they provide a way around the lost wakeup problem. The current implementation in cpython was submitted by me :) The source material is provided for

[issue11618] Locks broken wrt timeouts on Windows

sbt added the comment: > If we are rolling our own instead of using Semaphores (as has been > suggested for performance reasons) then using a Condition variable is > IMHO safer than a custom solution because the correctness of that > approach is so easily provable. Assuming that you trust th

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > I'm just providing this as a fast alternative to the Semaphore, which > as far as I know, will cause a kernel call every time. A Semaphore might be "slow", but I'm not sure other primitives are faster. For the record, I tried another implementation using a cr

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: I'm just providing this as a fast alternative to the Semaphore, which as far as I know, will cause a kernel call every time. Complicated is relative. In terms of the condition variable api, I wouldn't say that it is. But given the fact that we have

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > Yes, the race condition with the timeout is a problem. > Here is a patch that implements this lock using a condition variable. > I agree that one must consider performance/simplicity when doing this. I don't understand why you need something that complicated.

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson added the comment: Yes, the race condition with the timeout is a problem. Here is a patch that implements this lock using a condition variable. I agree that one must consider performance/simplicity when doing this. -- Added file: http://bugs.python.org/file21322/l

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > > At that time, the Pythread_* functions were still in use by the GIL > > implementation, and it made a difference judging by the commit message. > > Hmm. And if some application uses thread.lock heavily, won't it still > make a difference? An acquire/releas

[issue11618] Locks broken wrt timeouts on Windows

Martin v. Löwis added the comment: > At that time, the Pythread_* functions were still in use by the GIL > implementation, and it made a difference judging by the commit message. Hmm. And if some application uses thread.lock heavily, won't it still make a difference? -- __

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > Interestingly, it used to be a Semaphore up to [5e6e9e893acd]; in > [cde4da18c4fa], Yakov Markovitch rewrote this to be the faster > implementation we have today. At that time, the Pythread_* functions were still in use by the GIL implementation, and it made

[issue11618] Locks broken wrt timeouts on Windows

Martin v. Löwis added the comment: Interestingly, it used to be a Semaphore up to [5e6e9e893acd]; in [cde4da18c4fa], Yakov Markovitch rewrote this to be the faster implementation we have today. -- ___ Python tracker

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > Have you tried benchmarking it? > > Interlocked functions are *much* faster than Win32 mutex/semaphores in > the uncontended case. Well, I'd rather have obviously correct code than difficult-to-understand "speedy" code. The patch I've posted takes less th

[issue11618] Locks broken wrt timeouts on Windows

sbt added the comment: Have you tried benchmarking it? Interlocked functions are *much* faster than Win32 mutex/semaphores in the uncontended case. It only doubles the time taken for a "l.acquire(); l.release()" loop in Python code, but at the C level it is probably 10 times slower. Do

[issue11618] Locks broken wrt timeouts on Windows

Changes by Antoine Pitrou : -- nosy: +brian.curtin, tim.golden ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: Well, Windows 2000 has semaphores, so why not use them? It makes the code much simpler. Patch attached (including test). -- nosy: +loewis Added file: http://bugs.python.org/file21308/semlocknt.patch ___ Python track

[issue11618] Locks broken wrt timeouts on Windows

sbt added the comment: First stab at a fix. Gets rid of mutex->thread_id and adds a mutex->timeouts counter. Does not try to prevent mutex->owned from overflowing. When no timeouts have occurred I don't think it changes behaviour, and it uses the same number of Interlocked functions. --

[issue11618] Locks broken wrt timeouts on Windows

Antoine Pitrou added the comment: > It appears that the thread_id field is never actually used > -- is it there to help with debugging? Perhaps it should just be > removed. True, I think we can remove it. > does this mean that in Python 3.2 > threads are only supported with pthreads and win32

[issue11618] Locks broken wrt timeouts on Windows

Changes by Antoine Pitrou : -- nosy: +krisvale, pitrou ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http://m

[issue11618] Locks broken wrt timeouts on Windows

New submission from sbt : In thread_nt.h, when the WaitForSingleObject() call in EnterNonRecursiveMutex() fails with WAIT_TIMEOUT (or WAIT_FAILED) the mutex is left in an inconsistent state. Note that the first line of EnterNonRecursiveMutex() is the comment /* Assume that the thread waits