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
+
+#ifndefPy_GIL_DISABLED


Suggested change
#endif
#ifndefPy_GIL_DISABLED
#else/* Py_GIL_DISABLED */

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

Objects/dictobject.c

 returnNULL;
 }

+#endif


Suggested change
#endif
#endif/* Py_GIL_DISABLED */

colesbury

A few code style comments below

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

Objects/dictobject.c

+#ifdefPy_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
#ifdefPy_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

+staticint
+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-appbot 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 8d3080bCompare 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 locksFeb 6, 2024

DinoV force-pushed the nogil_dict_iter_threadsafe branch from 8d3080b to bf395f6Compare 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 0dd1a06Compare February 7, 2024 00:14

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

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

bedevere-appbot 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 resolvedHide resolved

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

methane reviewed Feb 16, 2024
View reviewed changes

Objects/dictobject.c Outdated
returnNULL;
}

#endif
Copy link
Member
methaneFeb 16, 2024
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
#endif
#endif/* Py_GIL_DISABLED */

colesbury reviewed Feb 21, 2024
View reviewed changes

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

colesbury approved these changes Feb 22, 2024
View reviewed changes

bedevere-appbot 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 a1d7718Compare February 22, 2024 18:32

DinoV merged commit 1002fbe into python:mainFeb 22, 2024

bedevere-appbot 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.

		staticint
		acquire_key_value(PyObject*key_loc, PyObjectvalue, PyObject**value_loc,

Uh oh!

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

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

Uh oh!

Conversation

DinoV commented Feb 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colesbury left a comment • 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!

Uh oh!

DinoV commented Feb 16, 2024

Uh oh!

methaneFeb 16, 2024

Choose a reason for hiding this comment

Uh oh!

methaneFeb 16, 2024

Choose a reason for hiding this comment

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

colesburyFeb 21, 2024

Choose a reason for hiding this comment

Uh oh!

colesburyFeb 21, 2024

Choose a reason for hiding this comment

Uh oh!

colesburyFeb 21, 2024

Choose a reason for hiding this comment

Uh oh!

colesburyFeb 21, 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!

colesburyFeb 22, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DinoV commented Feb 6, 2024•
edited
Loading

colesbury left a comment •
edited
Loading