Issue 25084: remove semi-busy loop in py2.7 threading.Condition.wait(timeout=x) (original) (raw)

Issue25084

Created on 2015-09-13 13:40 by flavio, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Messages (16)
msg250562 - (view)	Author: Flavio Grossi (flavio)	Date: 2015-09-13 13:40
threading.Condition.wait(timeout=x) is implemented in python 2 as a semi-busy loop which causes cpu wakeups and unexpected cpu use. I think this is somewhat problematic since it causes problems in all modules which use that method, such as Queue.get() when used with a timeout. This issue has been reported and "fixed" in red hat based distributions in a way which i find problematic, as detailed here: https://bugzilla.redhat.com/show_bug.cgi?id=1230802 The attached patch backports the following change from py3 to fix the problem: https://hg.python.org/cpython/rev/01d1fd775d16/
msg250712 - (view)	Author: STINNER Victor (vstinner) *	Date: 2015-09-14 22:34
As you noticed, this issue was correctly fixed in Python 3 by using the C timed acquire methods of locks, instead of polling. The problem is that Python 2 supports a wide range of threading models: Python/thread_atheos.h Python/thread_beos.h Python/thread_cthread.h Python/thread_lwp.h Python/thread_nt.h Python/thread_os2.h Python/thread_pth.h Python/thread_pthread.h Python/thread_sgi.h Python/thread_solaris.h Python/thread_wince.h In Python 3, it was possible to use more features of OS threads because we dropped all implementations to only keep the 2 major implementations: Python/thread_nt.h Python/thread_pthread.h Your change adds a new feature to threading.Lock in Python 2: -.. method:: Lock.acquire([blocking]) +.. method:: Lock.acquire(blocking=True, timeout=-1) But features cannot be added to Python 2 anymore! The backport changes critical C code. There is a (high) risk of introducing a regression. I suggest to close this issue as WONTFIX. @Benjamin (Python 2.7 release manager): What do you think? In 2015, it's time to upgrade to Python 3! https://docs.python.org/3/howto/pyporting.html
msg250727 - (view)	Author: Flavio Grossi (flavio)	Date: 2015-09-15 07:17
First of all, thank you for your support. I fully understand what you are saying, however, being stuck to python 2.7 because of libraries still not ported to 3, this is a serious problem to us, and, while i agree this would introduce a new "feature", it also fixes a bug which in some cases renders some functionalities (Queue.get() for example) not very usable as it is. Is there any chance this will be fixed? Or even having the remaining thread implementations fixed will lead to reject this? Thank you
msg250729 - (view)	Author: STINNER Victor (vstinner) *	Date: 2015-09-15 07:38
2015-09-15 9:17 GMT+02:00 Flavio Grossi <report@bugs.python.org>: > however, being stuck to python 2.7 because of libraries still not ported to 3, this is a serious problem to us, Are there private libraries or public libraries? If there are public, what are these libraries. I ported a lot of libraries last year, more and more libraries are compatible with Python 3.
msg250750 - (view)	Author: Flavio Grossi (flavio)	Date: 2015-09-15 09:07
>> however, being stuck to python 2.7 because of libraries > Are there private libraries or public libraries? It is a mix of public and internal libraries with the main public blockers being twisted and apache thrift (and other libs which have py3 alternatives but don't have retrocompatible apis). (Yes, twisted is almost supported, but still has some parts not ready for python 3)
msg250752 - (view)	Author: Alyssa Coghlan (ncoghlan) *	Date: 2015-09-15 10:08
For the sake of folks writing single-source code, and needing to support Python 2.7 for at least as long as we're supporting it upstream, I believe it would be beneficial to have consistency here. For those that didn't follow the Fedora/RHEL issue chain, the original Fedora 19 bug report was https://bugzilla.redhat.com/show_bug.cgi?id=917709 and the corresponding "won't fix" upstream issue was issue 17748. Unfortunately, in addition to only helping in a subset of cases, the workaround we applied also extends the affected APIs in an undocumented way that's incompatible with the Python 3 changes to the same API, so I've suggested that regardless of the verdict upstream, we should bring the API extension into line with Python 3 downstream.
msg250757 - (view)	Author: Antoine Pitrou (pitrou) *	Date: 2015-09-15 11:29
There's no consistency issue here, Python 3 just shows better performance and resource usage. Python 2 has always had the busy polling implementation.
msg250981 - (view)	Author: STINNER Victor (vstinner) *	Date: 2015-09-18 13:22
Nick, Antoine: do you agree to close this issue as WONTFIX for the reasons explained in ?
msg251000 - (view)	Author: Antoine Pitrou (pitrou) *	Date: 2015-09-18 14:42
Yes.
msg251050 - (view)	Author: Alyssa Coghlan (ncoghlan) *	Date: 2015-09-19 03:56
The consistency issue I was referring to related to the "timeout" argument - we currently have a patch applied to Fedora (and derivatives) that instead adds a "balanced=True" flag argument so you can pass "polling=False" to turn off the busy loop (at the risk of a long wake-up delay). This means the current situation we have in Fedora (et al) is: * Python 2 only code can pass "balanced=False" (but probably wouldn't want to due to the potential increase in wake-up latency) * Python 3 only code can specify a timeout directly * Single source Python 2/3 code can't do either (since the API signatures are different) So downstream I'll be advocating in favour of Flavio's pthread patch regardless of what we do upstream - that way hybrid Python 2/3 code that's specific to the Fedora derived ecosystem (and we have a lot of that in the operating system layer) can eventually be updated to use threading timeouts while remaining consistent between Fedora (which is switching the system Python to Python 3) and RHEL/CentOS 7 (which will continue to use Python 2.7). For upstream, I think it would be desirable to backport the timeout argument and simply have it throw RuntimeError if used with any of the backends other than thread_nt and thread_pthread. That way single source Python 2/3 code gains access to the timeout option, while code relying on one of the no-longer-available-in-Python-3 threading backends doesn't incur any stability risks. However, I don't feel strongly enough about that to argue in favour of it against opposition - I'm happy enough to confine my argument to changing the downstream Fedora/RHEL/CentOS patch.
msg251062 - (view)	Author: STINNER Victor (vstinner) *	Date: 2015-09-19 08:01
About the new optional balanced parameter added downstream to Fedora, the patch is very small compared to timedlock.patch. It only changes the Python code, not the C code: --- /home/haypo/prog/python/2.7/Lib/threading.py 2014-11-05 15:05:11.432003853 +0100 +++ /usr/lib64/python2.7/threading.py 2015-07-05 16:16:00.000000000 +0200 @@ -306,7 +306,7 @@ else: return True - def wait(self, timeout=None): + def wait(self, timeout=None, balancing=True): """Wait until notified or until a timeout occurs. If the calling thread has not acquired the lock when this method is @@ -355,7 +355,10 @@ remaining = endtime - _time() if remaining <= 0: break - delay = min(delay * 2, remaining, .05) + if balancing: + delay = min(delay * 2, remaining, 0.05) + else: + delay = remaining _sleep(delay) if not gotit: if __debug__: (with some additional changes to pass balanced parameter) Compare: $ diffstat balanced.patch threading.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) $ diffstat timedlock.patch Doc/library/thread.rst	31 ++++++++++--- Doc/library/threading.rst	32 +++++++++++++- Include/pythread.h	37 ++++++++++++++++ Lib/dummy_thread.py	8 +++ Lib/multiprocessing/pool.py	6 +- Lib/test/lock_tests.py	44 +++++++++++++++++-- Lib/threading.py	25 +++-------- Modules/threadmodule.c	53 +++++++++++++++++++---- Python/thread_nt.h	33 +++++++++++--- Python/thread_pthread.h	98 +++++++++++++++++++++++++++++++++++--------- 10 files changed, 296 insertions(+), 71 deletions(-) Do you know if the balanced parameter is used in the wild? Such code only works with Python 2 on Fedora/CentOS/RHEL, right? "For upstream, I think it would be desirable to backport the timeout argument and simply have it throw RuntimeError if used with any of the backends other than thread_nt and thread_pthread." It's not how Python handles portability. There are two choices in Python: * some functions are only available on some platforms, ex: os.fork() * the feature is supported by all platforms and Python uses a different implementation depending on the platform (ex: os.kill) Raising RuntimeError depending on the platform is not a good practice. "That way single source Python 2/3 code gains access to the timeout option, while code relying on one of the no-longer-available-in-Python-3 threading backends doesn't incur any stability risks." This issue is about the performance of Condition.wait(). The timeout parameter already exists on Python 2 in Condition.wait(). So it's already possible to write a single code base compatible with Python 2 and Python 3. If you are talking about adding e *new* timeout parameter to threading.Lock, it's a new feature. Usually, we don't add new features in minor releases. It makes maintenace more complex: you cannot guarantee anymore that an application written for Python 2.7.x will work on Python 2.7.y. If you really want this, I consider that a PEP is required to define exactly the scope of such change. "However, I don't feel strongly enough about that to argue in favour of it against opposition - I'm happy enough to confine my argument to changing the downstream Fedora/RHEL/CentOS patch." For this specific change (optimizing Condition.wait(timeout) using OS timeout), it makes sense to have a downstream only patch because the change will be specific to Linux. Well, I'm not strongly opposed to optimize Python 2, but since the patch is not portable, if you really want it, I consider that the topic should be discussed on python-dev to have a wide audience.
msg251069 - (view)	Author: Flavio Grossi (flavio)	Date: 2015-09-19 10:00
> About the new optional balanced parameter added downstream to Fedora, the patch is very small compared to timedlock.patch. It only changes the Python code, not the C code The balancing fix has 3 main problems imo: - It causes long delays to receive the notification (i.e. with a timeout of 30s the caller may be notified after 30s in the worst case) - It doesn't apply to other affected APIs, e.g. Queue.get() which uses a condition internally. - It only fixes the problem in python programs which explicitly use it (and being redhat specific i guess it is not very used)
msg251074 - (view)	Author: STINNER Victor (vstinner) *	Date: 2015-09-19 11:34
"The balancing fix has 3 main problems imo:" Oh by the way, I don't want to apply it to Python 2.7 upstream for a similar reason: it's a new feature which would be added to a Python 2.7 minor version. We try to avoid that, even if Python 2.7 is special.
msg251075 - (view)	Author: Antoine Pitrou (pitrou) *	Date: 2015-09-19 11:39
Ok, let's close it then.
msg251164 - (view)	Author: Alyssa Coghlan (ncoghlan) *	Date: 2015-09-20 11:33
+1 - after the further discussion, addressing this downstream as a patch specifically to the pthread backend sounds good to me.
msg251207 - (view)	Author: STINNER Victor (vstinner) *	Date: 2015-09-21 06:55
Nick Coghlan added the comment: > +1 - after the further discussion, addressing this downstream as a patch specifically to the pthread backend sounds good to me. Cool, I like when we agree :-) @Flavio Grossi: Sorry for you, but it's time to upgrade to Python 3. Twisted made great progress on Python 3 support. For Apache Thrift, maybe you can help them to port the library to Python 3?