gh-112075: Make PyDictKeysObject thread-safe by DinoV · Pull Request #114741 · python/cpython · GitHub

Adds locking for shared keys

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

How about adding a macro for the free-threading version and the default version?

I did similar approach at listobject: 393cbef

See: _Py_SET_ITEMREF

@corona10 is there a significant difference in performance between the two builds? I'm not sure if it is worth it to add these preprocessor guards everywhere if there is no measurable effect :) Is there a different motivation than performance?

Regardless of whether we do the macro approach everywhere, I think it's a good idea in this specific case (specifically for performance).

This mostly looks good to me.

I don't think we want to be locking around shared_keys_usable_size(). It's both not sufficient for thread-safety at most of the call sites and not what we want to be doing for performance reasons.

We can use a weaker ordering here that will be faster, especially on x86 where "release" doesn't require any memory barrier:

    _Py_atomic_store_ssize_relaxed(&keys->dk_nentries, keys->dk_nentries + 1);
    _Py_atomic_store_ssize_release(&keys->dk_usable, keys->dk_usable - 1);

(I don't think we have _Py_atomic_store_ssize_release yet, though)

I find the memory orderings hard to reason correctly about, so I like to model them with CDSChecker. Here's the model I used for this:
https://github.com/colesbury/c11-model-checker/blob/cpython-models/test/gh-112075.c

One minor formatting comment, but otherwise LGTM

Suggested change

Adds locking for shared PyDictKeysObject's for dictionaries

bedevere-app Bot mentioned this pull request Jan 30, 2024

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

Closed

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

DinoV force-pushed the nogil_dict_pydictkeys branch 3 times, most recently from fc6beda to 137eebe Compare January 30, 2024 01:19

corona10 reviewed Jan 30, 2024

View reviewed changes

DinoV force-pushed the nogil_dict_pydictkeys branch from 137eebe to 0891529 Compare January 30, 2024 17:31

DinoV marked this pull request as ready for review January 30, 2024 18:05

DinoV requested review from markshannon and methane as code owners January 30, 2024 18:05

bedevere-app Bot added the awaiting core review label Jan 30, 2024

methane reviewed Jan 31, 2024

View reviewed changes

Comment thread Objects/dictobject.c Outdated

DinoV force-pushed the nogil_dict_pydictkeys branch from 0891529 to 398cb23 Compare January 31, 2024 17:04

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

colesbury reviewed Feb 1, 2024

View reviewed changes

Comment thread Objects/dictobject.c Outdated

Comment thread Objects/dictobject.c

Comment thread Objects/dictobject.c Outdated

Comment thread Objects/dictobject.c Outdated

Comment thread Objects/dictobject.c Outdated

Comment thread Objects/dictobject.c Outdated

corona10 mentioned this pull request Feb 2, 2024

gh-112050: Make collections.deque thread-safe in free-threaded builds #113830

Merged

DinoV force-pushed the nogil_dict_pydictkeys branch 4 times, most recently from fe12c49 to 87c3446 Compare February 6, 2024 23:33

colesbury reviewed Feb 7, 2024

View reviewed changes

colesbury reviewed Feb 8, 2024

View reviewed changes

Comment thread Include/cpython/pyatomic_msc.h Outdated

Comment thread Include/cpython/pyatomic_msc.h Outdated

Comment thread Objects/dictobject.c Outdated

Comment thread Objects/dictobject.c Outdated

DinoV force-pushed the nogil_dict_pydictkeys branch from a22f723 to 808dd89 Compare February 15, 2024 23:45

DinoV mentioned this pull request Feb 15, 2024

gh-112075: Accessing a single element should optimistically avoid locking #115109

Merged

colesbury approved these changes Feb 20, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting core review labels Feb 20, 2024

DinoV added 2 commits February 20, 2024 11:58

Make PyDictKeysObject thread-safe

4715734

Fix naming of unsplit -> combined

18d2916

DinoV force-pushed the nogil_dict_pydictkeys branch from 808dd89 to 4914ba8 Compare February 20, 2024 20:01

DinoV added 3 commits February 20, 2024 12:28

Lock less and deal with atomic updates to shared size

61caf84

Address review feedback

268c806

Lock in dictresize, not in callers

7407ce9

DinoV force-pushed the nogil_dict_pydictkeys branch 2 times, most recently from 90bea6a to 88ab576 Compare February 20, 2024 23:25

Fixup recursive locking

a9d3666

DinoV force-pushed the nogil_dict_pydictkeys branch from 88ab576 to a9d3666 Compare February 21, 2024 00:12

DinoV merged commit 176df09 into python:main Feb 21, 2024

bedevere-app Bot removed the awaiting merge label Feb 21, 2024

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024

pythongh-112075 : Make PyDictKeysObject thread-safe (python#114741 )

d577f63

Adds locking for shared PyDictKeysObject's for dictionaries

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024

pythongh-112075 : Make PyDictKeysObject thread-safe (python#114741 )

97f461b

Adds locking for shared PyDictKeysObject's for dictionaries

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

LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025

pythongh-112075 : Make PyDictKeysObject thread-safe (python#114741 )

eed7e93

Adds locking for shared PyDictKeysObject's for dictionaries

dpdani mentioned this pull request Jun 17, 2026

test_abc hangs on TSan Parallel Test on Free Threading #151593

Closed

                   _Py_DecRefTotal(_PyInterpreterState_GET());
               #endif
-                  if (--dk->dk_refcnt == 0) {
+                  if (_Py_atomic_add_ssize(&dk->dk_refcnt, -1) == 1) {

+                  if (_PyDict_HasSplitTable(mp)) {
+                      LOCK_KEYS(keys);
+                      dictkeys_incref(keys);
+                  } else {

		_Py_atomic_store_ssize(&keys->dk_nentries, keys->dk_nentries + 1);
		_Py_atomic_store_ssize(&keys->dk_usable, keys->dk_usable - 1);

gh-112075: Make PyDictKeysObject thread-safe by DinoV · Pull Request #114741 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-112075: Make PyDictKeysObject thread-safe - #114741

gh-112075: Make PyDictKeysObject thread-safe#114741DinoV merged 6 commits intopython:mainpython/cpython:mainfrom DinoV:nogil_dict_pydictkeysDinoV/cpython:nogil_dict_pydictkeysCopy head branch name to clipboard

Conversation

DinoV commented Jan 30, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

corona10 Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

colesbury Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colesbury Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

colesbury Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gh-112075: Make PyDictKeysObject thread-safe#114741
DinoV merged 6 commits into
python:mainfrom
DinoV:nogil_dict_pydictkeys

DinoV commented Jan 30, 2024 •
edited by bedevere-app Bot
Loading

corona10 Jan 30, 2024 •
edited
Loading