Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-112075: Make PyDictKeysObject thread-safe #114741

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

DinoV
Copy link
Contributor

@DinoV DinoV commented Jan 30, 2024

@@ -331,7 +350,7 @@ dictkeys_decref(PyInterpreterState *interp, PyDictKeysObject *dk)
#ifdef Py_REF_DEBUG
_Py_DecRefTotal(_PyInterpreterState_GET());
#endif
if (--dk->dk_refcnt == 0) {
if (_Py_atomic_add_ssize(&dk->dk_refcnt, -1) == 1) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@corona10 corona10 Jan 30, 2024

Choose a reason for hiding this comment

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

I did similar approach at listobject: 393cbef

See: _Py_SET_ITEMREF

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@DinoV DinoV marked this pull request as ready for review January 30, 2024 18:05
Objects/dictobject.c Outdated Show resolved Hide resolved
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

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.

Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Comment on lines 170 to 171
_Py_atomic_store_ssize(&keys->dk_nentries, keys->dk_nentries + 1);
_Py_atomic_store_ssize(&keys->dk_usable, keys->dk_usable - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Show resolved Hide resolved
Include/cpython/pyatomic_msc.h Outdated Show resolved Hide resolved
Include/cpython/pyatomic_msc.h Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

One minor formatting comment, but otherwise LGTM

if (_PyDict_HasSplitTable(mp)) {
LOCK_KEYS(keys);
dictkeys_incref(keys);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else {
}
else {

@DinoV DinoV force-pushed the nogil_dict_pydictkeys branch 2 times, most recently from 90bea6a to 88ab576 Compare February 20, 2024 23:25
@DinoV DinoV merged commit 176df09 into python:main Feb 21, 2024
33 checks passed
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
Adds locking for shared PyDictKeysObject's for dictionaries
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
Adds locking for shared PyDictKeysObject's for dictionaries
@DinoV DinoV deleted the nogil_dict_pydictkeys branch May 31, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants