-
-
Notifications
You must be signed in to change notification settings - Fork 31k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RLock's are SLOW #47251
Comments
threading.RLock acquire/release is very slow. A order of magnitude """ if __name__=="__main__" :
print RLockSpeed()
""" Result: |
You should investigate and try to diagnose where the speed difference |
Running with python -O the timing gets a little closer between Lock and and only consist on: Simple profiler dump: Ordered by: internal time, call count ncalls tottime percall cumtime percall filename:lineno(function) |
Le samedi 21 juin 2008 à 16:40 +0000, sebastian serrano a écrit :
One could always try to rewrite RLock by replacing calls to However, given the profile table you have appended, it will only save at |
As suggested by pitrou, I wrote an implementation of RLock in C.
Notes:
|
Oops, I forgot to update PyInit__Thread() with my new time:
Here is the new patch. |
Wow, that was quick. Did you try to replace threading.RLock with your By the way:
Removing the debugging statements is fine, but apart from that the C Speaking of which, it would be nice if RLock was subclassable. I don't |
I doubt subclassability of RLock matters but who knows, people do code Regardless, using a C version wrapped in a simple python container class Given the final 2.6 beta is scheduled for today, this won't make it into |
Gregory, would you have an advice on bpo-3618? |
I've recently done this to implement potential deadlock detection. I Hugh |
Here is a new patch against py3k. It passes all tests and is generally |
Reviewers: , http://codereview.appspot.com/150055/diff/1/4 http://codereview.appspot.com/150055/diff/1/4#newcode221 http://codereview.appspot.com/150055/diff/1/4#newcode246 Description: Please review this at http://codereview.appspot.com/150055 Affected files: |
rlock_acquire_doc: "(...) return None once the lock is acquired". rlock_release(): Is the assert(self->rlock_count > 0); really required? rlock_acquire_restore(): raise an error if the lock acquire failed, 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_new(): why not reusing type instead of Py_TYPE(self) in __exit__: rlock_release() is defined as __exit__() with METH_VARARGS, Anything, thanks for the faster RLock! If your patch is commited, you can reconsider bpo-3618 (possible deadlock |
Thanks for the review. I will make the suggested modifications. http://codereview.appspot.com/150055/diff/1/4 http://codereview.appspot.com/150055/diff/1/4#newcode221
Right. http://codereview.appspot.com/150055/diff/1/4#newcode246
We always check rlock_count before rlock_owner anyway but, yes, I could |
You're right, I should fix that docstring.
Right again :) I forgot this was unsigned.
For rlock_acquire(), I mimicked what the Python code (_PyRLock.acquire) (and I agree this is not necessarily right, because failing to lock if But you're right that the Python version of rlock_acquire_restore()
As I said to Gregory, the current code doesn't rely on rlock_owner to be
Good point.
I just mimicked the corresponding declarations in the non-recursive lock
Indeed. Thanks ! |
Here is an updated patch. I addressed all review comments, except the |
Can you make the C implementation's repr() show something similar to the |
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 in r76189. Thanks for the reviews, everyone. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: