Mailing List Archive: Python: Bugs

[issue11618] Locks broken wrt timeouts on Windows

Index | Next | Previous | View Threaded

report at bugs

Apr 19, 2012, 3:26 AM

Post #1 of 19 (121 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows

Kristján Valur Jónsson <kristjan [at] ccpgames> 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/condition variable structures, that can be used on windows to do other such things without resorting to explicit kernel objects.

This code works on XP and newer, since it relies on the "semaphore" kernel object being present. In addition, if compiled to target Vista or greater, it will use the built-in critical section primitives and the FRWLock objects (which are faster still than CriticalSection objects and more robust)

----------
status: pending -> open
Added file: http://bugs.python.org/file25271/ntlocks.patch

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 19, 2012, 5:30 AM

Post #2 of 19 (119 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Antoine Pitrou <pitrou [at] free> 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?

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 20, 2012, 7:58 AM

Post #3 of 19 (122 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Kristján Valur Jónsson <kristjan [at] ccpgames> 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()
1000000 loops, best of 3: 0.746 usec per loop

D:\pydev\hg\cpython2>pcbuild\amd64\python.exe -m timeit -s "import _thread; l = _thread.allocate_lock()" l.acquire();l.release()
1000000 loops, best of 3: 0.749 usec per loop

Two runs with CV locks (emulated)

D:\pydev\hg\cpython2>pcbuild\amd64\python.exe -m timeit -s "import _thread; l = _thread.allocate_lock()" l.acquire();l.release()
1000000 loops, best of 3: 0.278 usec per loop

D:\pydev\hg\cpython2>pcbuild\amd64\python.exe -m timeit -s "import _thread; l = _thread.allocate_lock()" l.acquire();l.release()
1000000 loops, best of 3: 0.279 usec per loop

Two runs with CV locks targeted for Vista:

D:\pydev\hg\cpython2>pcbuild\amd64\python.exe -m timeit -s "import _thread; l = _thread.allocate_lock()" l.acquire();l.release()
1000000 loops, best of 3: 0.272 usec per loop

D:\pydev\hg\cpython2>pcbuild\amd64\python.exe -m timeit -s "import _thread; l = _thread.allocate_lock()" l.acquire();l.release()
1000000 loops, best of 3: 0.272 usec per loop

You can see the big win from not doing kernel switches all the time. shedding 60% of the time.
Once in user space, moving from CriticalSection objects to SRWLock objects is less beneficial, being overshadowed by Python overhead. Still, 2% overall is not to be frowned upon.

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 23, 2012, 1:12 PM

Post #4 of 19 (115 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Kristján Valur Jónsson <kristjan [at] ccpgames> 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 the kernel transitions caused by the GIL being ticked....

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 23, 2012, 1:21 PM

Post #5 of 19 (118 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Antoine Pitrou <pitrou [at] free> 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 transitions caused
> by the GIL being ticked....

That's the old GIL. The new GIL uses a fixed timeout with a condition variable.

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 23, 2012, 2:05 PM

Post #6 of 19 (114 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Kristján Valur Jónsson <kristjan [at] ccpgames> 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 GIL will be living on for many a good year, just like 2.7 will.

But you make a good point. I had forgotten that the new GIL is actually implemented with emulated condition variables (current version contributed by myself :). I think a different patch is in order, where ceval_gil.h makes use of the platform specific "condition variable" services as declared in thread_platform.h. There is no point in duplicating code.

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 24, 2012, 2:14 PM

Post #7 of 19 (113 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Kristján Valur Jónsson <kristjan [at] ccpgames> 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

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 26, 2012, 2:11 PM

Post #8 of 19 (107 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Kristján Valur Jónsson <kristjan [at] ccpgames> added the comment:

So, what do you think, should this go in? Any qualms about the thread_nt_cv.h header?

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 26, 2012, 3:11 PM

Post #9 of 19 (106 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Antoine Pitrou <pitrou [at] free> 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 <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 26, 2012, 3:59 PM

Post #10 of 19 (104 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Martin v. Löwis <martin [at] v> 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 <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 27, 2012, 3:10 AM

Post #11 of 19 (107 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Kristján Valur Jónsson <kristjan [at] ccpgames> 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).

Incidentally, we should make sure that python defines NTDDI_VERSION to NTDDI_WINXP (0x05010000), either in the sources before including "windows" (tricky) or in the solution (probably in the .prefs files)

This will ensure that we don't attempt to use non-existent features, unless we dynamically check for them.

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 27, 2012, 7:14 AM

Post #12 of 19 (105 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Antoine Pitrou <pitrou [at] free> 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 detection, so that we can provide a single installer for all Windows versions.

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 27, 2012, 8:10 AM

Post #13 of 19 (106 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Kristján Valur Jónsson <kristjan [at] ccpgames> 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 request :)
I'm not sure we do it elsewhere in Python, and the benefit is doubtful...

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 27, 2012, 8:13 AM

Post #14 of 19 (104 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Brian Curtin <brian [at] python> 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 <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 27, 2012, 8:23 AM

Post #15 of 19 (106 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Kristján Valur Jónsson <kristjan [at] ccpgames> 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 patch was radical enough, tbh.)

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 30, 2012, 2:05 AM

Post #16 of 19 (97 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Martin v. Löwis <martin [at] v> 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 for API "forever". In 2.5, there was a check for presence of GetFileAttributesExA; in 2.4, there was a check for CryptAcquireContextA.

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

Apr 30, 2012, 3:03 AM

Post #17 of 19 (93 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Kristján Valur Jónsson <kristjan [at] ccpgames> added the comment:

Martin, I think you misunderstand completely. the patch is _not_ about using the VISTA features. It is about not using a "mutex" for threading.lock.

Currently, the locks in python use Mutex objects, and a WaitForSingleObjects() system call to acquire them.
This patch replaces theses locks with user-level objects (critical sections and condition variables.). This drops the time needed for an uncontended acquire/release by 60% since there is no kernel transition and scheduling.

The patch comes in two flavors. The current version _emulates_ condition variables on Windows by the same mechanism as I introduced for the new GIL, that is, using a combination of "critical section" objects and a construct made of a "semaphore" and a counter.

Also provided, for those that want, and for future reference, is a version that uses native system objects (windows condition variables and SRWLocks). I can drop them from the patch to make you happy, but they are dormant and nicely show how conditional compilation can switch in more modern features for a different target architecture.

K

> -----Original Message-----
> From: Martin v. Löwis [mailto:report [at] bugs]
> 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 <martin [at] v> 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 for API "forever". In 2.5, there was
> a check for presence of GetFileAttributesExA; in 2.4, there was a check for
> CryptAcquireContextA.
>
> ----------
>
> _______________________________________
> Python tracker <report [at] bugs>
> <http://bugs.python.org/issue11618>
> _______________________________________

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

May 2, 2012, 1:12 PM

Post #18 of 19 (88 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Kristján Valur Jónsson <kristjan [at] ccpgames> 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 more places.
2) Implement the "Lock" primitive in Python using CritialSection and condition variables, rather than windows Mutexes. This gives a large performance boost on uncontended locks.
3) Provide an alternate implementation of the Condition Variable for a build target of Vista/Server 2008, using the native contidion variable objects available for that platform.

I think Martin got distraught by 3) and though that was the only thing this patch is about. The important part is 1) and 2) whereas 3) is provided as a bonus (and to make sure that 1) is future-safe)

So, can we get this reviewed please?

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

report at bugs

May 4, 2012, 10:34 AM

Post #19 of 19 (93 views)
Permalink

[issue11618] Locks broken wrt timeouts on Windows [In reply to]

Antoine Pitrou <pitrou [at] free> 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
+#undef _PY_EMULATED_WIN_CV
+#define _PY_EMULATED_WIN_CV 1
+#endif

so am I right to understand that when compiled under Vista or later, it will produce an XP-incompatible binary?

----------

_______________________________________
Python tracker <report [at] bugs>
<http://bugs.python.org/issue11618>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads