threading.RLock acquire/release is very slow. A order of magnitudehigher than no reentrant threading.Lock:"""def RLockSpeed() :  import time, threading  t=time.time()  result={}  for i in xrange(1000000) :    pass  result["empty loop"]=time.time()-t  l=threading.Lock()  t=time.time()  for i in xrange(1000000) :    l.acquire()    l.release()  result["Lock"]=time.time()-t  l=threading.RLock()  t=time.time()  for i in xrange(1000000) :    l.acquire()    l.release()  result["RLock"]=time.time()-t  return resultif __name__=="__main__" :  print RLockSpeed()"""Result:{'empty loop': 0.074212074279785156, 'RLock': 10.144084215164185,'Lock': 1.2489800453186035}

You should investigate and try to diagnose where the speed differencecomes from. ISTM the RLock class is implemented in Python while the Lockclass is simply an alias to the builtin native lock type, which couldexplain most of the discrepancy.

Running with python -O the timing gets a little closer between Lock andRLock. 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.720541000366time RLock 4.90231084824         400004 function calls in 0.982 CPU seconds   Ordered by: internal time, call count   ncalls  tottime  percall  cumtime  percall filename:lineno(function)   100000    0.304    0.000    0.390    0.000 threading.py:116(acquire)   100000    0.278    0.000    0.360    0.000 threading.py:136(release)        1    0.232    0.232    0.982    0.982 rlock.py:27(testRLock)   200000    0.168    0.000    0.168    0.000threading.py:788(current_thread)        1    0.000    0.000    0.000    0.000 threading.py:103(__init__)        1    0.000    0.000    0.000    0.000 threading.py:98(RLock)        1    0.000    0.000    0.000    0.000 threading.py:76(__init__)        0    0.000             0.000          profile:0(profiler)

Le samedi 21 juin 2008 à 16:40 +0000, sebastian serrano a écrit :> sebastian serrano <sebastian@devsar.com> 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 tothreading.current_thread() with thread.get_ident().However, given the profile table you have appended, it will only save atmost 30% of the time. If someone needs a more important speed-up, heshould reimplement the RLock type in C (and contribute it back :-)).

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 keywordNotes: - 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

Oops, I forgot to update PyInit__Thread() with my new time: - Add PyType_Ready() - Register RLockType to threading dictHere is the new patch.

Wow, that was quick. Did you try to replace threading.RLock with yourimplementation, 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 stateRemoving the debugging statements is fine, but apart from that the Cimplementation should mimick the current behaviour. Even if thisbehaviour has potential pitfalls.Speaking of which, it would be nice if RLock was subclassable. I don'tknow whether any software relies on this though.

I doubt subclassability of RLock matters but who knows, people do codethings.Regardless, using a C version wrapped in a simple python container classthat calls the underlying C implementation's methods should besufficient to allow sub-classing.Given the final 2.6 beta is scheduled for today, this won't make it into2.6/3.0 so we've got some time to polish up what we want.

Gregory, would you have an advice on#3618?

> 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 - seehttp://www.mjmwired.net/kernel/Documentation/lockdep-design.txtHugh

Here is a new patch against py3k. It passes all tests and is generally10-15x faster than the pure Python version.

Reviewers: ,http://codereview.appspot.com/150055/diff/1/4FileModules/_threadmodule.c (right):http://codereview.appspot.com/150055/diff/1/4#newcode221Modules/_threadmodule.c:221: return PyBool_FromLong((long) r);This explicit (long) cast is unnecessary.http://codereview.appspot.com/150055/diff/1/4#newcode246Modules/_threadmodule.c:246: PyThread_release_lock(self->rlock_lock);reset self->rlock_owner to 0 before releasing the lock.Description:code review forhttp://bugs.python.org/issue3001Please review this athttp://codereview.appspot.com/150055Affected files:   MLib/test/test_threading.py   MLib/threading.py   MModules/_threadmodule.c

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 inrlock_acquire() (especially if blocking is True)? Or if the error cannever 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, asrlock_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 deadlockin python IO implementation).

Thanks for the review. I will make the suggested modifications.http://codereview.appspot.com/150055/diff/1/4FileModules/_threadmodule.c (right):http://codereview.appspot.com/150055/diff/1/4#newcode221Modules/_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#newcode246Modules/_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 couldreset rlock_owner out of safety.http://codereview.appspot.com/150055

> 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 ifblocking is True would probably indicate a low-level system error, butthe 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 fromthe C code, although the rest of the method clearly assumes the lock hasbeen 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 bereset 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 lockobject. Apparently it's safe on all architectures Python has beenrunning 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 !

Here is an updated patch. I addressed all review comments, except theone about acquire_restore() checking the return result of acquire(),because I think it's really a weakness in the Python implementation.

Can you make the C implementation's repr() show something similar to the Python implementation?

Yes, here is a new patch adding tp_repr.

rlock4.patch looks correct and pass test_threading.py tests.

I've committed the latest patch inr76189. Thanks for the reviews, everyone.