[issue4751] Patch for better thread support in hashlib

Gregory P. Smith added the comment: Committed with a couple refactorings in trunk r72267. I also added a test (basically checking for corruption that would occur if the locks weren't working). (I'll sort out any py3k vs trunk differences to make future change merges easier). -- status

[issue4751] Patch for better thread support in hashlib

Changes by Lukas Lueg : Removed file: http://bugs.python.org/file13057/hashlibopenssl_gil_py27.diff ___ Python tracker ___ ___ Python-bugs-list

[issue4751] Patch for better thread support in hashlib

Lukas Lueg added the comment: bump hashlibopenssl_gil_py27.diff has not yet been applied to py27 and does not apply cleanly any more. Here is an updated version. -- status: pending -> open Added file: http://bugs.python.org/file13646/hashlibopenssl_gil_py27_2.diff

[issue4751] Patch for better thread support in hashlib

Changes by Collin Winter : -- nosy: +collinwinter, jyasskin ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: @ebfe: Your patch is very close to r68411 (patch for py3k), and so it looks correct (I didn't test it). ___ Python tracker ___ _

[issue4751] Patch for better thread support in hashlib

Lukas Lueg added the comment: Patch for 2.7 Added file: http://bugs.python.org/file13057/hashlibopenssl_gil_py27.diff ___ Python tracker ___ _

[issue4751] Patch for better thread support in hashlib

Lukas Lueg added the comment: yes, I got lost on that one. I'll create a patch for 2.7 tonight. ___ Python tracker ___ ___ Python-bugs-list mai

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: @ebfe: Did you wrote the patch (for python 2.7)? Are you still interrested to write the patch? ___ Python tracker ___ __

[issue4751] Patch for better thread support in hashlib

Gregory P. Smith added the comment: assigning to me so i don't lose track of making sure this happens for trunk. -- assignee: -> gregory.p.smith components: +Extension Modules -Library (Lib) ___ Python tracker __

[issue4751] Patch for better thread support in hashlib

Lukas Lueg added the comment: I'll do a patch for 2.7 -- versions: +Python 2.7 -Python 3.1 ___ Python tracker ___ ___ Python-bugs-list

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: Committed to py3k in r68411. Please tell me if you intend to do a patch for 2.7. Otherwise, you/I can close the issue. -- stage: -> committed/rejected status: open -> pending ___ Python tracker

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: Haypo's last patch is ok. If you want it to be in 2.7 too, however, you'll have to provide another patch (I won't do it myself). -- resolution: -> accepted versions: -Python 2.7 ___ Python tracker

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: Oops, nevermind what I said. The GIL isn't released if obj->lock isn't there. ___ Python tracker ___ ___ Pyth

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: There is still a potential problem. Figure the following: - thread A executes ENTER_HASHLIB while lock is NULL; therefore, thread A has released the GIL and doesn't hold any lock - thread B enters EVP_update with a large buffer (it can be there, since A doens't

[issue4751] Patch for better thread support in hashlib

Lukas Lueg added the comment: PyThread_allocate_lock can fail without interference. object->lock will stay NULL and the GIL is simply not released. ___ Python tracker ___

[issue4751] Patch for better thread support in hashlib

Changes by Lukas Lueg : Removed file: http://bugs.python.org/file12587/hashlibopenssl_small_lock-4.diff ___ Python tracker ___ ___ Python-bugs-

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: Updated patch: - change HASHLIB_GIL_MINSIZE to 2048 bytes - update hashlib documentation: add a note about the 2048 GIL limit - write a small test just for more sure that the GIL cases are tested (GIL released during object creation or on update) Added file

[issue4751] Patch for better thread support in hashlib

Gregory P. Smith added the comment: hashlibopenssl_small_lock-4.diff looks good to me. I also agree that HASHLIB_GIL_MINSIZE should be lowered to 2048. Commit it, and please backport it to trunk before closing this bug. -- nosy: -gps versions: +Python 2.7, Python 3.1 -Python 3.0 ___

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: The patch looks fine to me, apart from one point: the return value of PyThread_allocate_lock() should be checked for NULL, and the error either propagated or cleared. (I'd also suggest lowering HASHLIB_GIL_MINSIZE to 2048 or 4196) Gregory, what's your take? _

[issue4751] Patch for better thread support in hashlib

Changes by STINNER Victor : Removed file: http://bugs.python.org/file12554/hashlibopenssl_small_lock-3.patch ___ Python tracker ___ ___ Python-

[issue4751] Patch for better thread support in hashlib

Changes by STINNER Victor : Removed file: http://bugs.python.org/file12542/hashlibopenssl_small_lock-2.patch ___ Python tracker ___ ___ Python-

[issue4751] Patch for better thread support in hashlib

Lukas Lueg added the comment: I've modified haypo's patch as commented. The object's lock should be free 99.9% of the time so we try non-blocking first and can thereby skip releasing and re-locking the gil (to avoid a deadlock). Added file: http://bugs.python.org/file12587/hashlibopenssl_small

[issue4751] Patch for better thread support in hashlib

Changes by Lukas Lueg : Removed file: http://bugs.python.org/file12533/hashopenssl_threads-4.diff ___ Python tracker ___ ___ Python-bugs-list m

[issue4751] Patch for better thread support in hashlib

Lukas Lueg added the comment: The lock is created while having the GIL in EVP_update. No other function releases the GIL (besides the creator-function which does not need the local lock). Thereby no other thread can be in between ENTER and LEAVE while the lock is allocated. ___

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: I'm not sure about the approach of dynamically allocating self->lock. Imagine you allocate this lock while another thread is between ENTER_HASHLIB and LEAVE_HASHLIB. What happens on LEAVE_HASHLIB? The thread tries to release a lock it hadn't acquired (because th

[issue4751] Patch for better thread support in hashlib

ebfe added the comment: Haypo, we can probably reduce overhead by defining ENTER_HASHLIB like this: #define ENTER_HASHLIB(obj) \ if ((obj)->lock) { \ if (!PyThread_acquire_lock((obj)->lock, 0)) { \ Py_BEGIN_ALLOW_THREADS \ PyThread_acquire_lock((obj)->lock, 1

[issue4751] Patch for better thread support in hashlib

Changes by ebfe : Removed file: http://bugs.python.org/file12557/md5module_small_locks.diff ___ Python tracker ___ ___ Python-bugs-list mailing

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: ebfe> Here is another patch, this time for the fallback-md5-module Please open a separated issue for each module, this issue is already too long and complex ;-) And it would be easier to fix other modules when patches for hashlib will be accepted ;-) ___

[issue4751] Patch for better thread support in hashlib

ebfe added the comment: Here is another patch, this time for the fallback-md5-module. I know that situations are rare where openssl is not present but threading is. However they might occur out there and the md5module needed some love anyway: - The MD5 class from the fallback module can now als

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: > haypo, the patch will not compile when WITH_THREADS is not defined. Ooops, fixed (patch version 3). Added file: http://bugs.python.org/file12554/hashlibopenssl_small_lock-3.patch ___ Python tracker

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: About HASHLIB_GIL_MINSIZE, I'm unable to mesure the overhead. I tried timeit with 8190 and 8200 but the results are *very* close. I'm running Linux, it's maybe different on other OS. ___ Python tracker

[issue4751] Patch for better thread support in hashlib

ebfe added the comment: haypo, the patch will not compile when WITH_THREADS is not defined. The 'lock'-member in the object structure is not present without WITH_THREADS however the line 'if (self->lock == NULL && view.len >= HASHLIB_GIL_MINSIZE)' will always refer to it. __

[issue4751] Patch for better thread support in hashlib

ebfe added the comment: I don't think so. The interface should stay simple - python has very few such magic knobs. People will optimize for their own box as you said - and that code will run worse on all the others... Besides, we've lived so long with single-threaded openssl. Let's make HASHLI

[issue4751] Patch for better thread support in hashlib

Gregory P. Smith added the comment: First: thanks for doing this. I've had a patch sitting in my own sandbox to release the GIL while hashing for a while but I hadn't finished testing it. It looks pretty similar to what you've done so lets go with the patch being developed in this issue. Rat

[issue4751] Patch for better thread support in hashlib

Changes by STINNER Victor : Removed file: http://bugs.python.org/file12541/hashlibopenssl_small_lock.patch ___ Python tracker ___ ___ Python-bu

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: Update small lock patch: replace all tabs by spaces! I forget a change between Python trunk and my patch: there is also the error message for Unicode object. Before: >>> import hashlib; hashlib.md5("abc") TypeError: object supporting the buffer API requi

[issue4751] Patch for better thread support in hashlib

Changes by STINNER Victor : Removed file: http://bugs.python.org/file12459/hashopenssl_threads-2.diff ___ Python tracker ___ ___ Python-bugs-li

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: New implementation of finer lock grain in _hashlibopenssl: only create the lock at the first update with more than 8 KB bytes. Object creation/deallocation is faster if we hash less than 8 KB. Changes between hashopenssl_threads-4.diff and my new patch: fix t

[issue4751] Patch for better thread support in hashlib

ebfe added the comment: I don't think this is actually worth the trouble. You run into situation where one thread might decide that it needs a lock now with other threads being in the to-be-locked-area at that time. ___ Python tracker

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: > Releasing the GIL is somewhat expensive and should be avoided > if possible. Another possible solution is to create a lockless object by default, and create a lock if the data size is bigger than N (eg. 8 KB). When the lock is created, update will always u

[issue4751] Patch for better thread support in hashlib

ebfe added the comment: gnarf, actually it should be 'threads.append(Hasher(md))' in the script :-\ ___ Python tracker ___ ___ Python-bugs-list

[issue4751] Patch for better thread support in hashlib

Changes by ebfe : Removed file: http://bugs.python.org/file12461/hashopenssl_threads-3.diff ___ Python tracker ___ ___ Python-bugs-list mailing

[issue4751] Patch for better thread support in hashlib

ebfe added the comment: test-script Added file: http://bugs.python.org/file12534/hashlibtest2.py ___ Python tracker ___ ___ Python-bugs-list m

[issue4751] Patch for better thread support in hashlib

ebfe added the comment: Releasing the GIL is somewhat expensive and should be avoided if possible. I've moved LEAVE_HASHLIB in EVP_update so the object gets unlocked before we call Py_END_ALLOW_THREADS. This is *only* possible because EVP_update does not use the object beyond those lines. Here

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: The right fix would probably be to define ENTER_HASHLIB(self) as Py_BEGIN_ALLOW_THREADS PyThread_acquire_lock(self->lock) Py_END_ALLOW_THREADS ___ Python tracker ___

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: Ooooh, I suggested to ebfe to remove the GIL unlock/lock, but I was wrong :-( I hate locks! What is the right fix? Replace ENTER_HASHLIB(self) Py_BEGIN_ALLOW_THREADS ... Py_END_ALLOW_THREADS LEAVE_HASHLIB(self) by Py_BEGIN_ALLOW_THREADS ENT

[issue4751] Patch for better thread support in hashlib

Changes by STINNER Victor : ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: Actually, your code can deadlock since ENTER_HASHLIB doesn't release the GIL. Think about it: // Thread A is here, holding the GIL and waiting for self->lock to be // released by thread B ENTER_HASHLIB(self) Py_BEGIN_ALLOW_THREADS // Thread B is here, holding

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: Based on quick testing on my computer, we could probably put the limit as low as 1KB. But it may be that locks are cheap under Linux. In any case, the patch looks good, but I'm no OpenSSL expert. ___ Python tracker

[issue4751] Patch for better thread support in hashlib

Changes by Antoine Pitrou : -- nosy: +gps ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: hashlibtest.py results on my Quad Core with 4 threads: - unpatched: best=13.0 sec - patched: best=3.25 sec Some maths: 13.0 / 4 = 3.25 \o/ ___ Python tracker

[issue4751] Patch for better thread support in hashlib

ebfe added the comment: Here is another simple benchmarker. For me it shows almost perfect scaling (2 cores = 196% performance) if the buffer put into .update() is large enough. I deliberately did not move Py_BEGIN_ALLOW_THREADS into EVP_hash as we might call this function without having some l

[issue4751] Patch for better thread support in hashlib

Changes by STINNER Victor : Removed file: http://bugs.python.org/file12462/md5sum.py ___ Python tracker ___ ___ Python-bugs-list mailing list U

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: New version of my md5sum.py program limited to 10 threads. New benchmark with 160 files (size in 7..10 MB): - Python unpatched: best=4.8 sec - C version (/usr/bin/md5sum): best=3.6 sec - Python patched: best=2.1 sec As everybody knows, Python is faster than

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: > I've taken on haypo's patch to release the GIL only > when the buffer is larger than 10kb You can factorize the code by moving Py_BEGIN_ALLOW_THREADS / Py_END_ALLOW_THREADS *into* EVP_hash ;-) 10 KB is a random value or the fast value for your computer? I

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: > Is it save to assume that a buffer-view will not change until release() > is called - for all types supporting the buffer protocol in py3k ?? Yes, it is! ___ Python tracker _

[issue4751] Patch for better thread support in hashlib

ebfe added the comment: Thanks for the advices. Antoine, maybe you could clarify the situation regarding buffer-locks for me. In older versions of PEP 3118 the PyBUF_LOCK flag was still present but it doesn't seem to have made it's way into the final draft. Is it save to assume that a buffer-vi

[issue4751] Patch for better thread support in hashlib

Changes by ebfe : Removed file: http://bugs.python.org/file12453/hashopenssl_threads.diff ___ Python tracker ___ ___ Python-bugs-list mailing l

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: New version of ebfe's patch: - ENTER/LEAVE_HASHLIB: * don't touch GIL in ENTER_HASHLIB (it's useless) * add mandatory argument (explicit use of "self") - EVP_hash(): * restore Py_SAFE_DOWNCAST * simplify the code: always use the while() instead of

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: If view.len is negative, EVP_hash() may read invalid memory :-/ Be careful of integer overflow in this block: Py_ssize_t offset = 0, sublen = len; while (sublen) { unsigned int process = sublen > MUNCH_SIZE ? MUNCH_SIZE : sublen; ... } Y

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: EVP_copy() and EVP_get_digest_size() should call ENTER_HASHLIB/LEAVE_HASHLIB to protect self->ctx. ___ Python tracker ___ __

[issue4751] Patch for better thread support in hashlib

Antoine Pitrou added the comment: Hi, Very good idea. However, you don't need to discriminate for the bytes type specifically. When a buffer is taken on the object (with PyObject_GetBuffer()), the object is internally "locked" until the buffer is release with PyBuffer_Release(). Try with a byte

[issue4751] Patch for better thread support in hashlib

STINNER Victor added the comment: I think that you don't use Py_BEGIN_ALLOW_THREADS / Py_END_ALLOW_THREADS correctly: the GIL can be released when the hashlib lock is acquired (to run hash functions in parallel threads). So the macros should be: #define ENTER_HASHLIB \ PyThread_acquire_lo

[issue4751] Patch for better thread support in hashlib

New submission from ebfe : The hashlib functions provided by _hashopenssl.c hold the GIL all the time although the underlying openssl-library is basically thread-safe. I've attached a patch (svn diff) which basically does four things: * If python is compiled with thread-support, the EVPobject is