gh-112075: Iterating a dict shouldn't require locks #115108

DinoV · 2024-02-06T22:07:37Z

Makes iteration of a dict be lock free for the forward iteration case.

Handles races against the dict as well as allowing the iterator to be used from multiple threads simultaneously.

Includes some of the shared object marking from #115109

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

colesbury

I think we want to avoid expensive atomic operations, like atomic compare exchange and atomic increments, as well as avoiding locking.

This means giving up some atomicity for list and dict iterators compared to the GIL behavior. We should still avoid crashes/memory corruption, but I think it's okay for concurrent calls to next(it) on the same iterator object to return the same object. These iterators are almost always used by only a single thread and the performance cost of making the next atomic is relatively high.

See #114843 for the list iterator changes.

Objects/dictobject.c

DinoV · 2024-02-16T00:43:16Z

I think we want to avoid expensive atomic operations, like atomic compare exchange and atomic increments, as well as avoiding locking.

This means giving up some atomicity for list and dict iterators compared to the GIL behavior. We should still avoid crashes/memory corruption, but I think it's okay for concurrent calls to next(it) on the same iterator object to return the same object. These iterators are almost always used by only a single thread and the performance cost of making the next atomic is relatively high.

I was worried some crazy person might be using these things to distribute work across threads :P. I'm happy to make relax the guarantees and see how that goes. I suppose if that ever becomes an issue we can deal with it then :)

methane · 2024-02-16T07:29:51Z

Objects/dictobject.c

+#endif
+
+#ifndef Py_GIL_DISABLED


Suggested change

#endif

#ifndef Py_GIL_DISABLED

#else /* Py_GIL_DISABLED */

methane · 2024-02-16T07:30:49Z

Objects/dictobject.c

@@ -4558,6 +4607,8 @@ dictiter_iternextkey_lock_held(PyDictObject *d, PyObject *self)
    return NULL;
 }

+#endif


Suggested change

#endif

#endif /* Py_GIL_DISABLED */

colesbury

A few code style comments below

colesbury · 2024-02-21T17:56:11Z

Objects/dictobject.c

+#ifdef Py_GIL_DISABLED
+    if (has_unique_reference(result)) {
+#else
+    if (Py_REFCNT(result) == 1) {
+#endif


has_unique_reference already has special cases for Py_GIL_DISABLED and the default build:

Suggested change

#ifdef Py_GIL_DISABLED

if (has_unique_reference(result)) {

#else

if (Py_REFCNT(result) == 1) {

#endif

if (has_unique_reference(result)) {

colesbury · 2024-02-21T17:57:46Z

Objects/dictobject.c

+        if (values == NULL)
+            goto concurrent_modification;


I think the preferred style is to always include braces in new C code

colesbury · 2024-02-21T17:57:50Z

Objects/dictobject.c

+        if (i >= used)
+            goto fail;


colesbury · 2024-02-21T17:59:53Z

Objects/dictobject.c

+    // Even though we hold the lock here we may still lose a race against
+    // a lock-free iterator, therefore we may end up retrying our iteration.
+retry:
+    start_pos = i = _Py_atomic_load_ssize_relaxed(&di->di_pos);


Maybe use the wrappers from pycore_pyatomic_ft_wrappers.h to reduce the number of #ifdef statements.

Objects/dictobject.c

colesbury

This looks good, but I think it'd be better if acquire_key_value follows the -1/0 convention. The comment above acquire_key_value would need to be updated too.

colesbury · 2024-02-22T18:01:15Z

Objects/dictobject.c

+static int
+acquire_key_value(PyObject **key_loc, PyObject *value, PyObject **value_loc,


Also here: -1 for error and 0 for success

…values seen

) Makes iteration of a dict be lock free for the forward iteration case.

bedevere-app bot mentioned this pull request Feb 6, 2024

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

Closed

DinoV added skip news topic-free-threading labels Feb 6, 2024

DinoV force-pushed the nogil_dict_iter_threadsafe branch 2 times, most recently from 1266799 to 8d3080b Compare February 6, 2024 22:47

DinoV changed the title ~~gh-112075: Accessing a single element should optimistically avoid locking~~ gh-112075: Iterating a dict shouldn't require locks Feb 6, 2024

DinoV force-pushed the nogil_dict_iter_threadsafe branch from 8d3080b to bf395f6 Compare February 6, 2024 22:54

DinoV mentioned this pull request Feb 6, 2024

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

Merged

DinoV force-pushed the nogil_dict_iter_threadsafe branch 3 times, most recently from 2b2e75a to 0dd1a06 Compare February 7, 2024 00:14

DinoV marked this pull request as ready for review February 7, 2024 00:48

DinoV requested review from methane and markshannon as code owners February 7, 2024 00:48

bedevere-app bot added the awaiting core review label Feb 7, 2024

DinoV requested a review from colesbury February 7, 2024 20:39

colesbury reviewed Feb 8, 2024

View reviewed changes

Objects/dictobject.c Outdated Show resolved Hide resolved

DinoV force-pushed the nogil_dict_iter_threadsafe branch from 0dd1a06 to 3c03084 Compare February 16, 2024 00:43

methane reviewed Feb 16, 2024

View reviewed changes

colesbury reviewed Feb 21, 2024

View reviewed changes

DinoV force-pushed the nogil_dict_iter_threadsafe branch 3 times, most recently from e2a13a2 to 0941e62 Compare February 21, 2024 22:06

colesbury approved these changes Feb 22, 2024

View reviewed changes

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

DinoV added 2 commits February 22, 2024 10:09

Thread safe lock-free iteration

afddb70

Don't use atomics for iteratio and allow multiple threads to race on …

39bc495

…values seen

DinoV added 5 commits February 22, 2024 10:09

Fixup ifdef/ifndef

902ce67

Use has_unique_reference

0ba2f5d

Less atomics

042cf26

Fix style issues

a079a6d

acquire_key_value returns -1 and 0

a1d7718

DinoV force-pushed the nogil_dict_iter_threadsafe branch from 0941e62 to a1d7718 Compare February 22, 2024 18:32

DinoV merged commit 1002fbe into python:main Feb 22, 2024
33 checks passed

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

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

pythongh-112075: Iterating a dict shouldn't require locks (python#115108

13bd0c0

) Makes iteration of a dict be lock free for the forward iteration case.

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

pythongh-112075: Iterating a dict shouldn't require locks (python#115108

3b06e72

) Makes iteration of a dict be lock free for the forward iteration case.

sobolevn mentioned this pull request May 9, 2024

gh-118849: Fix "code will never be executed" warning in dictobject.c #118850

Merged

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

pythongh-112075: Iterating a dict shouldn't require locks (python#115108

9b49cc5

) Makes iteration of a dict be lock free for the forward iteration case.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-112075: Iterating a dict shouldn't require locks #115108

gh-112075: Iterating a dict shouldn't require locks #115108

DinoV commented Feb 6, 2024 •

edited

Loading

colesbury left a comment •

edited

Loading

DinoV commented Feb 16, 2024

methane Feb 16, 2024

methane Feb 16, 2024

colesbury left a comment

colesbury Feb 21, 2024

colesbury Feb 21, 2024

colesbury Feb 21, 2024

colesbury Feb 21, 2024

colesbury left a comment

colesbury Feb 22, 2024

		static int
		acquire_key_value(PyObject *key_loc, PyObject value, PyObject **value_loc,

gh-112075: Iterating a dict shouldn't require locks #115108

gh-112075: Iterating a dict shouldn't require locks #115108

Conversation

DinoV commented Feb 6, 2024 • edited Loading

colesbury left a comment • edited Loading

Choose a reason for hiding this comment

DinoV commented Feb 16, 2024

methane Feb 16, 2024

Choose a reason for hiding this comment

methane Feb 16, 2024

Choose a reason for hiding this comment

colesbury left a comment

Choose a reason for hiding this comment

colesbury Feb 21, 2024

Choose a reason for hiding this comment

colesbury Feb 21, 2024

Choose a reason for hiding this comment

colesbury Feb 21, 2024

Choose a reason for hiding this comment

colesbury Feb 21, 2024

Choose a reason for hiding this comment

colesbury left a comment

Choose a reason for hiding this comment

colesbury Feb 22, 2024

Choose a reason for hiding this comment

DinoV commented Feb 6, 2024 •

edited

Loading

colesbury left a comment •

edited

Loading