gh-135239: simpler use of mutex in hashlib & co #135267
Conversation
picnixz
added
skip news
awaiting core review
type-refactor
…135239 # Conflicts: # Modules/_hashopenssl.c # Modules/blake2module.c
picnixz
force-pushed
the
perf/hashlib/mutex-135239
branch
from
bb237c2 to
db57278
Compare
|
I think this is a nice refactoring. I haven't tested performances yet but I doubt it'll degrade by much. And even if it does degrade a bit, I'd rather make use of an unconditional lock rather than hoping that HACL* lib is threadsafe (which I don't know and maybe it's not easy to do it). |
|
Hum. We're loosing quite a lot of speed if we do incremental hashing with small buffers (we are 2x-3x slower). However, even with small buffers, we probably already have a race condition right, assuming that HACL* interface is not thread-safe (which I think it's not as the state doesn't have hold a mutex). |
|
Ok, so we're actually losing speed because of Am I right here @ZeroIntensity ? |
|
Ok, I managed to restore the perfs! |
|
I think we're good now. I'm leaving my dev session and won't be back until Thursday so this will need to wait. |
That sounds fine to me, but make sure that the locking is done in a consistent order. Note that acquiring the lock will release the GIL, unless you explicitly tell it not to with |
| do { \ | ||
| (obj)->mutex = (PyMutex){0}; \ | ||
| (obj)->use_mutex = true; \ | ||
| #define PyObject_HASHLIB_HEAD \ |
There was a problem hiding this comment.
Would it be possible to make it private? Rename it to _PyObject_HASHLIB_HEAD.
There was a problem hiding this comment.
Strictly speaking, it's an undefined behavior to have an underscore followed by a capital letter, but since we have so many _Py* in the codebase, I can either rename it as you suggested or HASHLIB_OBJECT_HEAD. Which one do you prefer (I can also add some _Py* prefixes to other macros but we historically had only HASHLIB_* prefixes).
There was a problem hiding this comment.
I'm fine with HASHLIB_OBJECT_HEAD.
| } | ||
| return PyUnicode_FromStringAndSize(digest_hex, sizeof(digest_hex)); | ||
| HASHLIB_RELEASE_LOCK(self); | ||
| return _Py_strhex((const char *)digest, MD5_DIGESTSIZE); |
There was a problem hiding this comment.
This change is unrelated and should be done in a separated PR.
There was a problem hiding this comment.
I'll isolate it when I'm offline. I won't merge this PR before the end of the week anyway
I've taken the liberty of normalizing code style. I'll do the same in other modules, (SHA and BLAKE2). That way, I'll never need to touch cosmetics again in crypto-modules. Well, if it's too much I can drop the last commit.