[issue3001] RLock's are SLOW
Antoine Pitrou pit...@free.fr added the comment: I've committed the latest patch in r76189. Thanks for the reviews, everyone. -- resolution: - fixed status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
STINNER Victor victor.stin...@haypocalc.com added the comment: rlock4.patch looks correct and pass test_threading.py tests. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
STINNER Victor victor.stin...@haypocalc.com added the comment: rlock_acquire_doc: (...) return None once the lock is acquired. That's wrong, acquire() always return a boolean (True or False). rlock_release(): Is the assert(self-rlock_count 0); really required? You're checking its value few lines before. rlock_acquire_restore(): raise an error if the lock acquire failed, whereas rlock_acquire() doesn't. Why not raising an error in rlock_acquire() (especially if blocking is True)? Or if the error can never occur, remove the error checking in rlock_acquire_restore(). rlock_acquire_restore(): (maybe) set owner to 0 if count is 0. rlock_release_save(): may also reset self-rlock_owner to 0, as rlock_release(). rlock_new(): why not reusing type instead of Py_TYPE(self) in Py_TYPE(self)-tp_free(self)? __exit__: rlock_release() is defined as __exit__() with METH_VARARGS, but it has no argument. Can it be a problem? Anything, thanks for the faster RLock! If your patch is commited, you can reconsider #3618 (possible deadlock in python IO implementation). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Antoine Pitrou pit...@free.fr added the comment: Thanks for the review. I will make the suggested modifications. http://codereview.appspot.com/150055/diff/1/4 File Modules/_threadmodule.c (right): http://codereview.appspot.com/150055/diff/1/4#newcode221 Modules/_threadmodule.c:221: return PyBool_FromLong((long) r); On 2009/11/07 07:48:05, gregory.p.smith wrote: This explicit (long) cast is unnecessary. Right. http://codereview.appspot.com/150055/diff/1/4#newcode246 Modules/_threadmodule.c:246: PyThread_release_lock(self-rlock_lock); On 2009/11/07 07:48:05, gregory.p.smith wrote: reset self-rlock_owner to 0 before releasing the lock. We always check rlock_count before rlock_owner anyway but, yes, I could reset rlock_owner out of safety. http://codereview.appspot.com/150055 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Antoine Pitrou pit...@free.fr added the comment: rlock_acquire_doc: (...) return None once the lock is acquired. That's wrong, acquire() always return a boolean (True or False). You're right, I should fix that docstring. rlock_release(): Is the assert(self-rlock_count 0); really required? You're checking its value few lines before. Right again :) I forgot this was unsigned. rlock_acquire_restore(): raise an error if the lock acquire failed, whereas rlock_acquire() doesn't. Why not raising an error in rlock_acquire() (especially if blocking is True)? For rlock_acquire(), I mimicked what the Python code (_PyRLock.acquire) does: if locking fails, it returns False instead. It is part of the API. (and I agree this is not necessarily right, because failing to lock if blocking is True would probably indicate a low-level system error, but the purpose of the patch is not to change the API) But you're right that the Python version of rlock_acquire_restore() doesn't check the return code either, so I may remove this check from the C code, although the rest of the method clearly assumes the lock has been taken. rlock_acquire_restore(): (maybe) set owner to 0 if count is 0. rlock_release_save(): may also reset self-rlock_owner to 0, as rlock_release(). As I said to Gregory, the current code doesn't rely on rlock_owner to be reset but, yes, we could still add it out of safety. rlock_new(): why not reusing type instead of Py_TYPE(self) in Py_TYPE(self)-tp_free(self)? Good point. __exit__: rlock_release() is defined as __exit__() with METH_VARARGS, but it has no argument. Can it be a problem? I just mimicked the corresponding declarations in the non-recursive lock object. Apparently it's safe on all architectures Python has been running on for years, although I agree it looks strange. If your patch is commited, you can reconsider #3618 (possible deadlock in python IO implementation). Indeed. Thanks ! -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Antoine Pitrou pit...@free.fr added the comment: Here is an updated patch. I addressed all review comments, except the one about acquire_restore() checking the return result of acquire(), because I think it's really a weakness in the Python implementation. -- Added file: http://bugs.python.org/file15284/rlock3.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Gregory P. Smith g...@krypto.org added the comment: Can you make the C implementation's repr() show something similar to the Python implementation? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Antoine Pitrou pit...@free.fr added the comment: Yes, here is a new patch adding tp_repr. -- Added file: http://bugs.python.org/file15285/rlock4.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Gregory P. Smith g...@krypto.org added the comment: Reviewers: , http://codereview.appspot.com/150055/diff/1/4 File Modules/_threadmodule.c (right): http://codereview.appspot.com/150055/diff/1/4#newcode221 Modules/_threadmodule.c:221: return PyBool_FromLong((long) r); This explicit (long) cast is unnecessary. http://codereview.appspot.com/150055/diff/1/4#newcode246 Modules/_threadmodule.c:246: PyThread_release_lock(self-rlock_lock); reset self-rlock_owner to 0 before releasing the lock. Description: code review for http://bugs.python.org/issue3001 Please review this at http://codereview.appspot.com/150055 Affected files: M Lib/test/test_threading.py M Lib/threading.py M Modules/_threadmodule.c -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Changes by Antoine Pitrou pit...@free.fr: -- assignee: - pitrou components: -Interpreter Core stage: patch review - needs patch versions: +Python 3.2 -Python 2.7, Python 3.1 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Changes by Daniel Diniz aja...@gmail.com: -- stage: - patch review ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Changes by Kevin Watters [EMAIL PROTECTED]: -- nosy: +kevinwatters ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Hugh Gibson [EMAIL PROTECTED] added the comment: I doubt subclassability of RLock matters but who knows, people do code things. I've recently done this to implement potential deadlock detection. I keep a record of the sequences of acquired locks, find unique sequences, then check for conflicts between each sequence. There's not much overhead and it highlighted some potential deadlocks where lock A and B were acquired AB in one route through code and BA in another route. The algorithm is a simplified version of that used in Linux - see http://www.mjmwired.net/kernel/Documentation/lockdep-design.txt Hugh -- nosy: +hgibson50 ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Changes by STINNER Victor [EMAIL PROTECTED]: Removed file: http://bugs.python.org/file11172/rlock.patch ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
STINNER Victor [EMAIL PROTECTED] added the comment: As suggested by pitrou, I wrote an implementation of RLock in C. Changes to Python version: - no debug message: i leave the message in #if 0 ... #endif - acquire() method argument blocking is not a keyword Notes: - RLock has no docstring! - In the Python version, RLock._release_save() replaces owner and counter attributes before release the lock. But releasing the lock may fails and no the object is in an inconsistent state -- keywords: +patch nosy: +haypo Added file: http://bugs.python.org/file11172/rlock.patch ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
STINNER Victor [EMAIL PROTECTED] added the comment: Oops, I forgot to update PyInit__Thread() with my new time: - Add PyType_Ready() - Register RLockType to threading dict Here is the new patch. Added file: http://bugs.python.org/file11175/rlock-v2.patch ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Antoine Pitrou [EMAIL PROTECTED] added the comment: Wow, that was quick. Did you try to replace threading.RLock with your implementation, and run the tests? By the way: - acquire() method argument blocking is not a keyword - In the Python version, RLock._release_save() replaces owner and counter attributes before release the lock. But releasing the lock may fails and no the object is in an inconsistent state Removing the debugging statements is fine, but apart from that the C implementation should mimick the current behaviour. Even if this behaviour has potential pitfalls. Speaking of which, it would be nice if RLock was subclassable. I don't know whether any software relies on this though. ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Gregory P. Smith [EMAIL PROTECTED] added the comment: I doubt subclassability of RLock matters but who knows, people do code things. Regardless, using a C version wrapped in a simple python container class that calls the underlying C implementation's methods should be sufficient to allow sub-classing. Given the final 2.6 beta is scheduled for today, this won't make it into 2.6/3.0 so we've got some time to polish up what we want. -- versions: +Python 2.7, Python 3.1 ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Antoine Pitrou [EMAIL PROTECTED] added the comment: Gregory, would you have an advice on #3618? -- versions: -Python 2.6, Python 3.0 ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Changes by Gregory P. Smith [EMAIL PROTECTED]: -- components: +Interpreter Core, Library (Lib) nosy: +gregory.p.smith priority: - normal ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
sebastian serrano [EMAIL PROTECTED] added the comment: Running with python -O the timing gets a little closer between Lock and RLock. This code won't be easy to improve in performance. The heaviest call is current_thread(), used at lines: 117:me = current_thread() 137:if self.__owner is not current_thread(): and only consist on: 788: def current_thread(): 789: try: 790: return _active[_get_ident()] 791: except KeyError: 792: ##print current_thread(): no current thread for, _get_ident() 793: return _DummyThread() Simple profiler dump: $../python-trunk/python -O rlock.py time Lock 0.720541000366 time RLock 4.90231084824 44 function calls in 0.982 CPU seconds Ordered by: internal time, call count ncalls tottime percall cumtime percall filename:lineno(function) 100.3040.0000.3900.000 threading.py:116(acquire) 100.2780.0000.3600.000 threading.py:136(release) 10.2320.2320.9820.982 rlock.py:27(testRLock) 200.1680.0000.1680.000 threading.py:788(current_thread) 10.0000.0000.0000.000 threading.py:103(__init__) 10.0000.0000.0000.000 threading.py:98(RLock) 10.0000.0000.0000.000 threading.py:76(__init__) 00.000 0.000 profile:0(profiler) -- nosy: +sserrano ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Antoine Pitrou [EMAIL PROTECTED] added the comment: Le samedi 21 juin 2008 à 16:40 +, sebastian serrano a écrit : sebastian serrano [EMAIL PROTECTED] added the comment: Running with python -O the timing gets a little closer between Lock and RLock. This code won't be easy to improve in performance. The heaviest call is current_thread(), used at lines: 117:me = current_thread() 137:if self.__owner is not current_thread(): One could always try to rewrite RLock by replacing calls to threading.current_thread() with thread.get_ident(). However, given the profile table you have appended, it will only save at most 30% of the time. If someone needs a more important speed-up, he should reimplement the RLock type in C (and contribute it back :-)). ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Antoine Pitrou [EMAIL PROTECTED] added the comment: You should investigate and try to diagnose where the speed difference comes from. ISTM the RLock class is implemented in Python while the Lock class is simply an alias to the builtin native lock type, which could explain most of the discrepancy. -- nosy: +pitrou ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Changes by Giampaolo Rodola' [EMAIL PROTECTED]: -- nosy: +giampaolo.rodola ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
New submission from Jesús Cea Avión [EMAIL PROTECTED]:
threading.RLock acquire/release is very slow. A order of magnitude
higher than no reentrant threading.Lock:
def RLockSpeed() :
import time, threading
t=time.time()
result={}
for i in xrange(100) :
pass
result[empty loop]=time.time()-t
l=threading.Lock()
t=time.time()
for i in xrange(100) :
l.acquire()
l.release()
result[Lock]=time.time()-t
l=threading.RLock()
t=time.time()
for i in xrange(100) :
l.acquire()
l.release()
result[RLock]=time.time()-t
return result
if __name__==__main__ :
print RLockSpeed()
Result:
{'empty loop': 0.074212074279785156, 'RLock': 10.144084215164185,
'Lock': 1.2489800453186035}
--
messages: 67497
nosy: jcea
severity: normal
status: open
title: RLock's are SLOW
type: performance
versions: Python 2.6, Python 3.0
___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3001
___
___
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3001] RLock's are SLOW
Changes by Adam Olsen [EMAIL PROTECTED]: -- nosy: +Rhamphoryncus ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue3001 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
