gh-112075: Add critical sections for most dict APIs #114508

DinoV · 2024-01-23T22:55:48Z

Starts adding thread safety to dict objects.

Adds critical sections around most APIs. Doesn't add any thread safety to the get-path as that'll be covered later.

There's generally 3 variations in here:

Use @critical_section for APIs which are exposed via argument clinic and don't directly correlate with a public C API which needs to acquire the lock
Use a _lock_held suffix for keeping changes to complicated functions simple and just wrapping them with a critical section
Acquire and release the lock in an existing function where it won't be overly disruptive to the existing logic

Most things are made thread safe, I think the one glaring exception is that fetches of a single item are not thread safe. I'm hoping we can get the QSBR support in sooner rather than later and get that working. This does make iteration thread safe but it requires a lock which is not the long term plan so eventually that should become lock-free.

This specifically doesn't address the other issues for dict thread safety called out in the issue, it's just addressing the critical sections.

Issue: Make dict objects thread-safe in --disable-gil builds #112075

Objects/dictobject.c

colesbury

This generally looks good. I included a number of formatting suggestions. Take them or leave them as you wish.

Objects/dictobject.c

colesbury · 2024-02-05T17:29:45Z

Objects/dictobject.c

+dictiter_iternextkey(PyObject *self)
+{
+    dictiterobject *di = (dictiterobject *)self;
+    PyDictObject *d = di->di_dict;


We'll need a bit more work to make concurrent next(it) thread-safe on the same iterator. That's not quite as urgent, but let's not lose track of it.

I actually have that ready to go I believe in what will become the PR for lcok-free thread-safe iteration, just need to get other things landed first :)

Include/internal/pycore_critical_section.h

Objects/dictobject.c

colesbury

LGTM

colesbury · 2024-02-06T21:06:30Z

Include/internal/pycore_critical_section.h

+#ifdef Py_DEBUG
+
+#define  _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op)                           \
+    if (Py_REFCNT(op) != 1) {                                                 \


Maybe adjust the whitespace before \

bedevere-bot · 2024-02-06T22:39:41Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86-64 macOS 3.x has failed when building commit 92abb01.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/366/builds/6553) and take a look at the build logs.
Check if the failure is related to this commit (92abb01) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/366/builds/6553

Failed tests:

test.test_multiprocessing_forkserver.test_processes

Summary of the results of the build (if available):

==

Click to see traceback logs

remote: Enumerating objects: 14, done.        
remote: Counting objects:   7% (1/13)        
remote: Counting objects:  15% (2/13)        
remote: Counting objects:  23% (3/13)        
remote: Counting objects:  30% (4/13)        
remote: Counting objects:  38% (5/13)        
remote: Counting objects:  46% (6/13)        
remote: Counting objects:  53% (7/13)        
remote: Counting objects:  61% (8/13)        
remote: Counting objects:  69% (9/13)        
remote: Counting objects:  76% (10/13)        
remote: Counting objects:  84% (11/13)        
remote: Counting objects:  92% (12/13)        
remote: Counting objects: 100% (13/13)        
remote: Counting objects: 100% (13/13), done.        
remote: Compressing objects:  25% (1/4)        
remote: Compressing objects:  50% (2/4)        
remote: Compressing objects:  75% (3/4)        
remote: Compressing objects: 100% (4/4)        
remote: Compressing objects: 100% (4/4), done.        
remote: Total 14 (delta 9), reused 10 (delta 9), pack-reused 1        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '92abb0124037e5bc938fa870461a26f64c56095b'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 92abb01240 gh-112075: Add critical sections for most dict APIs (#114508)
Switched to and reset branch 'main'

configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libpython3.13d.a(dynamic_annotations.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libpython3.13d.a(gc_free_threading.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libpython3.13d.a(jit.o) has no symbols
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups

make: *** [buildbottest] Error 2

bedevere-bot · 2024-02-06T22:59:00Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu NoGIL 3.x has failed when building commit 92abb01.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1225/builds/1373) and take a look at the build logs.
Check if the failure is related to this commit (92abb01) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1225/builds/1373

Failed tests:

test_tools
test_math

Summary of the results of the build (if available):

==

Click to see traceback logs

remote: Enumerating objects: 14, done.        
remote: Counting objects:   7% (1/13)        
remote: Counting objects:  15% (2/13)        
remote: Counting objects:  23% (3/13)        
remote: Counting objects:  30% (4/13)        
remote: Counting objects:  38% (5/13)        
remote: Counting objects:  46% (6/13)        
remote: Counting objects:  53% (7/13)        
remote: Counting objects:  61% (8/13)        
remote: Counting objects:  69% (9/13)        
remote: Counting objects:  76% (10/13)        
remote: Counting objects:  84% (11/13)        
remote: Counting objects:  92% (12/13)        
remote: Counting objects: 100% (13/13)        
remote: Counting objects: 100% (13/13), done.        
remote: Compressing objects:  25% (1/4)        
remote: Compressing objects:  50% (2/4)        
remote: Compressing objects:  75% (3/4)        
remote: Compressing objects: 100% (4/4)        
remote: Compressing objects: 100% (4/4), done.        
remote: Total 14 (delta 9), reused 10 (delta 9), pack-reused 1        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '92abb0124037e5bc938fa870461a26f64c56095b'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 92abb01240 gh-112075: Add critical sections for most dict APIs (#114508)
Switched to and reset branch 'main'

make: *** [Makefile:2101: buildbottest] Error 2

) Starts adding thread safety to dict objects. Use @critical_section for APIs which are exposed via argument clinic and don't directly correlate with a public C API which needs to acquire the lock Use a _lock_held suffix for keeping changes to complicated functions simple and just wrapping them with a critical section Acquire and release the lock in an existing function where it won't be overly disruptive to the existing logic

DinoV added topic-free-threading skip news labels Jan 23, 2024

DinoV changed the title ~~Nogil dict locks~~ gh-112075: Add critical sections for most dict APIs Jan 23, 2024

bedevere-app bot mentioned this pull request Jan 23, 2024

Make dict objects thread-safe in --disable-gil builds #112075

Closed

DinoV force-pushed the nogil_dict_locks branch 6 times, most recently from 937afe2 to 1b5b406 Compare January 24, 2024 18:59

DinoV marked this pull request as ready for review January 24, 2024 20:26

DinoV requested review from methane and markshannon as code owners January 24, 2024 20:26

bedevere-app bot added the awaiting core review label Jan 24, 2024

colesbury self-requested a review January 31, 2024 20:15

colesbury reviewed Jan 31, 2024

View reviewed changes

DinoV force-pushed the nogil_dict_locks branch from 1b5b406 to 8695859 Compare February 2, 2024 18:51

DinoV requested a review from rhettinger as a code owner February 2, 2024 18:51

DinoV force-pushed the nogil_dict_locks branch 2 times, most recently from 65dede8 to 4118b1d Compare February 2, 2024 23:07

rhettinger removed their request for review February 3, 2024 15:36

colesbury reviewed Feb 5, 2024

View reviewed changes

DinoV force-pushed the nogil_dict_locks branch 2 times, most recently from c326efa to d25c9a0 Compare February 6, 2024 19:21

colesbury approved these changes Feb 6, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Feb 6, 2024

DinoV added 3 commits February 6, 2024 13:39

Add macro to assert critical section is held

455461b

Critical sections

a0d1e27

Most dict operations should use locks

584f4cb

DinoV added 2 commits February 6, 2024 13:39

Lock someplaces outside of dicts & review feedback

aa80085

Fix formatting

ab5d224

DinoV force-pushed the nogil_dict_locks branch from d25c9a0 to ab5d224 Compare February 6, 2024 21:39

DinoV merged commit 92abb01 into python:main Feb 6, 2024
33 checks passed

bedevere-app bot removed the awaiting merge label Feb 6, 2024

DinoV deleted the nogil_dict_locks branch May 31, 2024 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-112075: Add critical sections for most dict APIs #114508

gh-112075: Add critical sections for most dict APIs #114508

DinoV commented Jan 23, 2024 •

edited

Loading

colesbury left a comment

colesbury Feb 5, 2024

DinoV Feb 5, 2024 •

edited

Loading

colesbury left a comment

colesbury Feb 6, 2024

bedevere-bot commented Feb 6, 2024

bedevere-bot commented Feb 6, 2024

gh-112075: Add critical sections for most dict APIs #114508

gh-112075: Add critical sections for most dict APIs #114508

Conversation

DinoV commented Jan 23, 2024 • edited Loading

colesbury left a comment

Choose a reason for hiding this comment

colesbury Feb 5, 2024

Choose a reason for hiding this comment

DinoV Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

colesbury left a comment

Choose a reason for hiding this comment

colesbury Feb 6, 2024

Choose a reason for hiding this comment

bedevere-bot commented Feb 6, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

bedevere-bot commented Feb 6, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

DinoV commented Jan 23, 2024 •

edited

Loading

DinoV Feb 5, 2024 •

edited

Loading