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
Patch for better thread support in hashlib #49001
Comments
The hashlib functions provided by _hashopenssl.c hold the GIL all the
I've tested this patch and did not run into problems. CPU occupancy |
I think that you don't use Py_BEGIN_ALLOW_THREADS / #define ENTER_HASHLIB \
PyThread_acquire_lock(self->lock, 1); \
Py_BEGIN_ALLOW_THREADS
#define LEAVE_HASHLIB \
Py_END_ALLOW_THREADS \
PyThread_release_lock(self->lock); If I'm right, issue bpo-4738 (zlib) is also affected. |
Hi, Very good idea. However, you don't need to discriminate for the bytes All in all, it should make your code and macros much simpler. |
EVP_copy() and EVP_get_digest_size() should call |
If view.len is negative, EVP_hash() may read invalid memory :-/ Be Py_ssize_t offset = 0, sublen = len;
while (sublen) {
unsigned int process = sublen > MUNCH_SIZE ? MUNCH_SIZE :
sublen;
...
} You removed Py_SAFE_DOWNCAST(len, Py_ssize_t, unsigned int) which Note: you might modify len directly instead of using a second variable |
New version of ebfe's patch:
Some rules for ENTER/LEAVE_HASHLIB:
About the GIL:
|
Thanks for the advices. Antoine, maybe you could clarify the situation regarding buffer-locks I've done some testing and the overhead of releasing and re-locking the |
Yes, it is! |
You can factorize the code by moving Py_BEGIN_ALLOW_THREADS / 10 KB is a random value or the fast value for your computer? I wrote a small benchmark: md5sum.py, my Python multithreaded version
My program creates N threads for N files, which is maybe stupid (eg. |
New version of my md5sum.py program limited to 10 threads. New
As everybody knows, Python is faster than the C language ;-) And the |
Here is another simple benchmarker. For me it shows almost perfect I deliberately did not move Py_BEGIN_ALLOW_THREADS into EVP_hash as we The 10kb limit was based on my own computer (MacBook Pro 2x2.5GHz) and |
hashlibtest.py results on my Quad Core with 4 threads:
Some maths: 13.0 / 4 = 3.25 \o/ |
Based on quick testing on my computer, we could probably put the limit |
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
ENTER_HASHLIB(self)
...
LEAVE_HASHLIB(self)
Py_END_ALLOW_THREADS
? |
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 |
Releasing the GIL is somewhat expensive and should be avoided if Here is a new patch and a small test-script. |
test-script |
gnarf, actually it should be 'threads.append(Hasher(md))' in the script :-\ |
Another possible solution is to create a lockless object by default, In general, you have two classes of hashlib usages:
When you have a small string, you don't need to release the GIL nor to |
I don't think this is actually worth the trouble. You run into situation |
New implementation of finer lock grain in _hashlibopenssl: only create Changes between hashopenssl_threads-4.diff and my new patch: fix the Changes between py3k trunk and my new patch:
|
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 required
after:
>>> import hashlib; hashlib.md5("abc")
TypeError: Unicode-objects must be encoded before hashing |
First: thanks for doing this. I've had a patch sitting in my own Rather than making HASHLIB_GIL_MINSIZE a constant I suggest making it a |
I don't think so. The interface should stay simple - python has very few such magic knobs. Besides, we've lived so long with single-threaded openssl. Let's make |
haypo, the patch will not compile when WITH_THREADS is not defined. The |
About HASHLIB_GIL_MINSIZE, I'm unable to mesure the overhead. I tried |
Ooops, fixed (patch version 3). |
Here is another patch, this time for the fallback-md5-module. I know
I might act on the sha modules as way the next days. sha256.c still I might a |
ebfe> Here is another patch, this time for the fallback-md5-module Please open a separated issue for each module, this issue is already |
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); \
Py_END_ALLOW_THREADS \
} \
} |
I'm not sure about the approach of dynamically allocating self->lock. |
The lock is created while having the GIL in EVP_update. No other Thereby no other thread can be in between ENTER and LEAVE while the lock |
I've modified haypo's patch as commented. The object's lock should be |
The patch looks fine to me, apart from one point: the return value of (I'd also suggest lowering HASHLIB_GIL_MINSIZE to 2048 or 4196) Gregory, what's your take? |
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. |
Updated patch:
|
PyThread_allocate_lock can fail without interference. object->lock will |
There is still a potential problem.
To remove this possibility, the macros should probably look like: #define ENTER_HASHLIB(obj) \
{ \
int __lock_exists = ((obj)->lock) != NULL; \
if (__lock_exists) { \
if (!PyThread_acquire_lock((obj)->lock, 0)) { \
Py_BEGIN_ALLOW_THREADS \
PyThread_acquire_lock((obj)->lock, 1); \
Py_END_ALLOW_THREADS \
} \
}
#define LEAVE_HASHLIB(obj) \
if (__lock_exists) { \
PyThread_release_lock((obj)->lock); \
} \
} |
Oops, nevermind what I said. The GIL isn't released if obj->lock isn't |
Haypo's last patch is ok. If you want it to be in 2.7 too, however, |
Committed to py3k in r68411. Please tell me if you intend to do a patch |
I'll do a patch for 2.7 |
assigning to me so i don't lose track of making sure this happens for |
@ebfe: Did you wrote the patch (for python 2.7)? Are you still |
yes, I got lost on that one. I'll create a patch for 2.7 tonight. |
Patch for 2.7 |
@ebfe: Your patch is very close to r68411 (patch for py3k), and so it |
bump hashlibopenssl_gil_py27.diff has not yet been applied to py27 and does |
Committed with a couple refactorings in trunk r72267. I also added a (I'll sort out any py3k vs trunk differences to make future change |
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: