GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111916: Make hashlib related modules thread-safe without the GIL by tomasr8 · Pull Request #111981 · python/cpython

Conversation

@tomasr8

@tomasr8 tomasr8 commented Nov 11, 2023 •
edited by gpshead
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Still a work in progress but I'd be happy for feedback ;)

I'm not entirely sure what to do about HASHLIB_GIL_MINSIZE. IIUC the original code avoids releasing the GIL and getting a separate lock when the update buffer is short enough. I'm guessing that in the nogil version, we'll have to lock the mutex regardless of the length (i.e. essentially getting rid of the macro)? Is that correct?

As a side note, I'm failing to build this PR with the GIL enabled. Running ./configure --with-pydebug and make -s -j2, I'm getting this error:

In file included from ./Modules/hashlib.h:7,
                 from ./Modules/md5module.c:29:
./Include/internal/pycore_lock.h: In function ‘PyMutex_LockFast’:
./Include/internal/pycore_lock.h:60:12: error: implicit declaration of function ‘_Py_atomic_compare_exchange_uint8’; did you mean ‘__atomic_compare_exchange_n’? [-Werror=implicit-function-declaration]
   60 |     return _Py_atomic_compare_exchange_uint8(lock_bits, &expected, _Py_LOCKED);

Even if I add #include "pyatomic.h" to pycore_lock.h I get the same error.. any idea why that is?

  • Issue: Make hashlib related modules thread-safe without the GIL #111916

Fixes #111916

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tomasr8
WIP: Make hashlib thread-safe without the GIL
bf79136
@bedevere-app bedevere-app Bot mentioned this pull request Nov 11, 2023
Make hashlib related modules thread-safe without the GIL #111916
Closed
colesbury
colesbury reviewed Nov 13, 2023
View reviewed changes

@colesbury colesbury left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build errors you're seeing are due to md5module.c being compiled with the limited API, which means internal-only functions like PyMutex_Lock are not available. You should remove these lines from md5module.c:

#ifndef _MSC_VER
#include "pyconfig.h"   // Py_NOGIL
#endif
#ifndef Py_NOGIL
// Need limited C API version 3.12 for Py_MOD_PER_INTERPRETER_GIL_SUPPORTED
#define Py_LIMITED_API 0x030c0000
#endif

And instead add:

#ifndef Py_BUILD_CORE_BUILTIN
#  define Py_BUILD_CORE_MODULE 1
#endif

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/_blake2/blake2s_impl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_blake2/blake2b_impl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_hashopenssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_hashopenssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_hashopenssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/hashlib.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/hashlib.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/sha3module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@colesbury

colesbury commented Nov 13, 2023

Copy link
Copy Markdown
Contributor

(I edited my review comment to note that you also should add Py_BUILD_CORE_MODULE to md5module.c)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 added 7 commits November 14, 2023 21:39
@tomasr8
Fix 'ENTER_HASHLIB' macro
00a1aa3
@tomasr8
Rename INIT_MUTEX
68e7934
@tomasr8
Fix 'use_mutex' logic
d04b2ec
@tomasr8
Fix build errors
bcb7fde
@tomasr8
Remove unnecessary #define
a4d816a
@tomasr8
Reinitialize mutex when copying an HMAC object
c8b6219
@tomasr8
Add news entry
c58160a
@tomasr8
tomasr8 marked this pull request as ready for review November 14, 2023 21:13
@tomasr8
tomasr8 requested a review from tiran as a code owner November 14, 2023 21:13
@bedevere-app bedevere-app Bot added the awaiting review label Nov 14, 2023
@colesbury colesbury added the topic-free-threading label Nov 14, 2023
@colesbury

colesbury commented Nov 14, 2023

Copy link
Copy Markdown
Contributor

It looks like some of the generated parts of Modules/_blake2/blake2s_impl.c are out of date. You'll need to re-run Argument Clinic. I think python3 Tools/clinic/clinic.py --make --exclude Lib/test/clinic.test.c --srcdir . should do the trick.
tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 added 2 commits November 15, 2023 18:20
@tomasr8
Re-add some comments
cfba913
@tomasr8
Rerun argument clinic
804318c
@tomasr8

tomasr8 commented Nov 15, 2023

Copy link
Copy Markdown
Member Author

Thanks for the thorough review! I reran AC as you suggested, so hopefully it should be good this time
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
colesbury requested a review from gpshead November 15, 2023 18:13
colesbury
colesbury approved these changes Nov 15, 2023
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @tomasr8! This LGTM.

@gpshead - would you be able to review this PR? (I saw your name on the git log for these files.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Nov 15, 2023
@gpshead gpshead self-assigned this Nov 15, 2023
gpshead
gpshead reviewed Nov 15, 2023
View reviewed changes
Comment thread Modules/md5module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead approved these changes Nov 15, 2023
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one minor style nit, overall looks great. thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Nov 15, 2023
@gpshead
style: add missing space.
f7c419f
@gpshead

gpshead commented Nov 15, 2023

Copy link
Copy Markdown
Member

the original code avoids releasing the GIL and getting a separate lock when the update buffer is short enough. I'm guessing that in the nogil version, we'll have to lock the mutex regardless of the length

Yep, the old HASHLIB_GIL_MINSIZE logic doesn't make sense in a free threaded build - always locking as this PR does works there. This PR could probably elide the concept from the code entirely during a free-threaded build, but that'd just makes the maintenance harder with additional ifdefs. For little value. It'll be an always-taken branch because use_mutex = true in free-threaded builds so the size will never get checked. Good enough - it won't have a measurable performance impact.
tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
gpshead enabled auto-merge (squash) November 15, 2023 23:37
@gpshead
gpshead merged commit a646560 into python:main Nov 15, 2023
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 15, 2023
@tomasr8
tomasr8 deleted the hashlib branch November 16, 2023 07:05
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@tomasr8 @aisk
pythongh-111916: Make hashlib related modules thread-safe without the…
fcc7a7e 
… GIL (python#111981)

Always use an individual lock on hash objects when in free-threaded builds.

Fixes python#111916
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@tomasr8 @Glyphack
pythongh-111916: Make hashlib related modules thread-safe without the…
a0c3bb8 
… GIL (python#111981)

Always use an individual lock on hash objects when in free-threaded builds.

Fixes python#111916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes
@colesbury colesbury colesbury approved these changes
@tiran tiran Awaiting requested review from tiran

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make hashlib related modules thread-safe without the GIL

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@tomasr8 @colesbury @gpshead
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.