Description

picnixz

opened on Jun 7, 2025

Issue body actions

Feature or enhancement

Proposal:

Currently, when doing incremental HMAC, if a message is sufficiently large, we lock the HMAC object but release the GIL so that we can call HMAC_Update without the GIL. However, for subsequent data, we will always do this. I'll need to estimate if multiple calls to HMAC, first with a very big data, then only with very small data, will be impacted. Usually, update() is called because we're in a chunk-based strategy. So, we could expect the next block of data to be as large as the one we're already hashing.

Nevertheless, I think we should turn off the mutex usage if we encounter a small block of data as we could have started with a very big chunk, then with very small ones (at least below the hardcoded limit).

I'll do some benchmarks tomorrow but I wanted to create an issue for that.

Note: a similar argument holds for hash functions and their update().

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Linked PRs

• gh-135239: simpler use of mutex in hashlib & co #135267

Activity

1. picnixz

   self-assigned this

   on Jun 7, 2025
2. picnixz

   added

   type-featureA feature request or enhancementA feature request or enhancement

   extension-modulesC modules in the Modules dirC modules in the Modules dir

   on Jun 7, 2025
3. picnixz

   changed the title [-]Smarter use of a mutex in incremental HMAC[/-] [+]Smarter use of a mutex in incremental HMAC and hash functions[/+] on Jun 8, 2025
4. picnixz commented on Jun 8, 2025

   

   picnixz

   on Jun 8, 2025

   MemberAuthor

   More actions

   Ok, my conclusions are that... it doesn't matter, except for blake2b where it's slightly slower to release/hold the GIL. But that's with an idle system. Maybe with a non-idle one it will be a bit worse. I'll make the adjustments won't be much slower:

       if (!self->use_mutex && buf.len >= HASHLIB_GIL_MINSIZE) {
           self->use_mutex = true;
       }
       if (self->use_mutex) {
           Py_BEGIN_ALLOW_THREADS
           PyMutex_Lock(&self->mutex);
           update(self->hash_state, buf.buf, buf.len);
           PyMutex_Unlock(&self->mutex);
           Py_END_ALLOW_THREADS
       } else {
           update(self->hash_state, buf.buf, buf.len);
       }

   Once we have self->use_mutex = true, the condition !self->use_mutex && buf.len >= HASHLIB_GIL_MINSIZE will always be false even if buf.len >= HASHLIB_GIL_MINSIZE is true. If we switch to a small data regime, then we'll likely do un-necessary PyMutex_Lock/PyMutex_Unlock calls, whereas we could just do the update at once. Having two comparisons + 1 LOAD + 1 STORE (to set self->use_mutex = false afterwards) will likely be faster than saving 1 comparison but keeping calls to PyMutex_*.

   I'll draft a PR to check if it's worth it or not, as it can also be considered a small refactoring.
5. picnixz

   added

   performancePerformance or resource usagePerformance or resource usage

   on Jun 8, 2025
6. picnixz commented on Jun 15, 2025

   

   picnixz

   on Jun 15, 2025

   MemberAuthor

   More actions

   Following the discussion in #135267 (comment), I've decided to always lock the object when we perform an update. The rationale is as follows:

   • AFAIK, the HACL* calls are not thread-safe in the first place.
   • Currently, we allow the object to be locked once we exceed a certain threshold.
   • Afterwards, the object will always use the mutex when dealing with updates.

   Considering that PyMutex is fast, I think we can unconditionally lock the object when we perform an update. It will considerably reduce maintenance cost as we will also avoid having this GIL_MINSIZE attribute, and we will also avoid this additional use_mutex flag and ENTER_HASHLIB/LEAVE_HASHLIB macros.
7. picnixz

   added

   type-refactorCode refactoring (with no changes in behavior)Code refactoring (with no changes in behavior)

   and removed

   type-featureA feature request or enhancementA feature request or enhancement

   on Jun 15, 2025
8. picnixz

   added a commit that references this issue on Jun 22, 2025

   gh-135239: simpler use of mutexes in cryptographic modules (#135267)

   e7295a8
9. picnixz

   closed this as completedon Jun 23, 2025
10. AndPuQing

    added a commit that references this issue on Jul 11, 2025

    pythongh-135239: simpler use of mutexes in cryptographic modules (pyt…

    f216c82
11. Pranjal095

    added a commit that references this issue on Jul 12, 2025

    pythongh-135239: simpler use of mutexes in cryptographic modules (pyt…

    2dfda94
12. taegyunkim

    added a commit that references this issue on Aug 4, 2025

    pythongh-135239: simpler use of mutexes in cryptographic modules (pyt…

    db05ae2
13. Agent-Hellboy

    added a commit that references this issue on Aug 19, 2025

    pythongh-135239: simpler use of mutexes in cryptographic modules (pyt…

    2b23cd6

Issue actions