Skip to content

gh-112075: Make some dict operations thread-safe without GIL #112247

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

gh-112075: Make some dict operations thread-safe without GIL #112247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c198b07
gh-112075: Make some `dict` operations thread-safe without GIL
chgnrdv Nov 18, 2023
fa6aada
make dict.__repr__ thread-safe with critical section API macro
chgnrdv Nov 18, 2023
12f184f
Merge branch 'main' into make-dict-ops-thread-safe-in-nogil
chgnrdv Nov 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions Objects/clinic/dictobject.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

110 changes: 77 additions & 33 deletions Objects/dictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ As a consequence of this, split keys have a maximum size of 16.
#include "pycore_call.h" // _PyObject_CallNoArgs()
#include "pycore_ceval.h" // _PyEval_GetBuiltin()
#include "pycore_code.h" // stats
#include "pycore_critical_section.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment like: #include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION()

#include "pycore_dict.h" // export _PyDict_SizeOf()
#include "pycore_gc.h" // _PyObject_GC_IS_TRACKED()
#include "pycore_object.h" // _PyObject_GC_TRACK(), _PyDebugAllocatorStats()
Expand Down Expand Up @@ -2505,15 +2506,18 @@ dict_repr(PyDictObject *mp)
PyObject *key = NULL, *value = NULL;
_PyUnicodeWriter writer;
int first;
PyObject *repr = NULL;

i = Py_ReprEnter((PyObject *)mp);
if (i != 0) {
return i > 0 ? PyUnicode_FromString("{...}") : NULL;
}

Py_BEGIN_CRITICAL_SECTION(mp);

if (mp->ma_used == 0) {
Py_ReprLeave((PyObject *)mp);
return PyUnicode_FromString("{}");
repr = PyUnicode_FromString("{}");
goto end;
Copy link
Contributor

Choose a reason for hiding this comment

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

I thin this may incorrectly call _PyUnicodeWriter_Dealloc() on an uninitialized writer below if repr is NULL (i.e., if PyUnicode_FromString returns an error).

For these functions with complex control flow, I think it's better to add a wrapper function than try to work Py_BEGIN_CRITICAL_SECTION() calls into the existing control flow. In other words:

  • Rename dict_repr to e.g. dict_repr_impl.
  • Add a new dict_repr that just calls dict_repr_impl under a Py_BEGIN_CRITICAL_SECTION() calls.

}

_PyUnicodeWriter_Init(&writer);
Expand All @@ -2522,7 +2526,7 @@ dict_repr(PyDictObject *mp)
writer.min_length = 1 + 4 + (2 + 4) * (mp->ma_used - 1) + 1;

if (_PyUnicodeWriter_WriteChar(&writer, '{') < 0)
goto error;
goto end;

/* Do repr() on each key+value pair, and insert ": " between them.
Note that repr may mutate the dict. */
Expand All @@ -2538,53 +2542,58 @@ dict_repr(PyDictObject *mp)

if (!first) {
if (_PyUnicodeWriter_WriteASCIIString(&writer, ", ", 2) < 0)
goto error;
goto end;
}
first = 0;

s = PyObject_Repr(key);
if (s == NULL)
goto error;
goto end;
res = _PyUnicodeWriter_WriteStr(&writer, s);
Py_DECREF(s);
if (res < 0)
goto error;
goto end;

if (_PyUnicodeWriter_WriteASCIIString(&writer, ": ", 2) < 0)
goto error;
goto end;

s = PyObject_Repr(value);
if (s == NULL)
goto error;
goto end;
res = _PyUnicodeWriter_WriteStr(&writer, s);
Py_DECREF(s);
if (res < 0)
goto error;
goto end;

Py_CLEAR(key);
Py_CLEAR(value);
}

writer.overallocate = 0;
if (_PyUnicodeWriter_WriteChar(&writer, '}') < 0)
goto error;

Py_ReprLeave((PyObject *)mp);
goto end;

return _PyUnicodeWriter_Finish(&writer);
repr = _PyUnicodeWriter_Finish(&writer);

error:
end:
Py_ReprLeave((PyObject *)mp);
_PyUnicodeWriter_Dealloc(&writer);
Py_XDECREF(key);
Py_XDECREF(value);
return NULL;
if (repr == NULL) {
_PyUnicodeWriter_Dealloc(&writer);
Py_XDECREF(key);
Py_XDECREF(value);
}
Py_END_CRITICAL_SECTION();
return repr;
}

static Py_ssize_t
dict_length(PyDictObject *mp)
{
#ifdef Py_NOGIL
return _Py_atomic_load_ssize_relaxed(&mp->ma_used);
#else
return mp->ma_used;
#endif
}

static PyObject *
Expand Down Expand Up @@ -2746,6 +2755,7 @@ dict_items(PyDictObject *mp)
}

/*[clinic input]
@critical_section
Copy link
Contributor

Choose a reason for hiding this comment

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

dict_fromkeys_impl should not have a critical_section. This locks the type object, which isn't necessary.

We may need critical sections for some code paths of _PyDict_FromKeys, but that will need to be within _PyDict_FromKeys.

@classmethod
dict.fromkeys
iterable: object
Expand All @@ -2757,7 +2767,7 @@ Create a new dictionary with keys from iterable and values set to value.

static PyObject *
dict_fromkeys_impl(PyTypeObject *type, PyObject *iterable, PyObject *value)
/*[clinic end generated code: output=8fb98e4b10384999 input=382ba4855d0f74c3]*/
/*[clinic end generated code: output=8fb98e4b10384999 input=8eef23b63d38c243]*/
{
return _PyDict_FromKeys((PyObject *)type, iterable, value);
}
Expand Down Expand Up @@ -2790,7 +2800,9 @@ dict_update_common(PyObject *self, PyObject *args, PyObject *kwds,
result = -1;
}
else if (arg != NULL) {
Py_BEGIN_CRITICAL_SECTION2(self, arg);
result = dict_update_arg(self, arg);
Py_END_CRITICAL_SECTION2();
}

if (result == 0 && kwds != NULL) {
Expand Down Expand Up @@ -3107,7 +3119,11 @@ _PyDict_MergeEx(PyObject *a, PyObject *b, int override)
static PyObject *
dict_copy(PyDictObject *mp, PyObject *Py_UNUSED(ignored))
{
return PyDict_Copy((PyObject*)mp);
PyObject *copy;
Py_BEGIN_CRITICAL_SECTION(mp);
copy = PyDict_Copy((PyObject*)mp);
Py_END_CRITICAL_SECTION();
return copy;
}

PyObject *
Expand Down Expand Up @@ -3316,7 +3332,9 @@ dict_richcompare(PyObject *v, PyObject *w, int op)
res = Py_NotImplemented;
}
else if (op == Py_EQ || op == Py_NE) {
Py_BEGIN_CRITICAL_SECTION2(v, w);
cmp = dict_equal((PyDictObject *)v, (PyDictObject *)w);
Py_END_CRITICAL_SECTION2();
if (cmp < 0)
return NULL;
res = (cmp == (op == Py_EQ)) ? Py_True : Py_False;
Expand Down Expand Up @@ -3511,7 +3529,9 @@ dict_setdefault_impl(PyDictObject *self, PyObject *key,
static PyObject *
dict_clear(PyDictObject *mp, PyObject *Py_UNUSED(ignored))
{
Py_BEGIN_CRITICAL_SECTION(mp);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need public APIs like PyDict_Clear() to be thread-safe. The critical sections need to be pushed down into that API.

I suggest doing something like in colesbury/nogil-3.12@d896dfc8db:

  • Rename the current implementation of PyDict_Clear() and make it static (e.g. static void _dict_clear(...))
  • Make PyDict_Clear() a simple wrapper around _dict_clear() that adds critical section calls.
  • Just call PyDict_Clear() from this function dict_clear(...).

PyDict_Clear((PyObject *)mp);
Py_END_CRITICAL_SECTION();
Py_RETURN_NONE;
}

Expand Down Expand Up @@ -3702,7 +3722,11 @@ _PyDict_KeysSize(PyDictKeysObject *keys)
static PyObject *
dict_sizeof(PyDictObject *mp, PyObject *Py_UNUSED(ignored))
{
return PyLong_FromSsize_t(_PyDict_SizeOf(mp));
PyObject *size;
Py_BEGIN_CRITICAL_SECTION(mp);
size = PyLong_FromSsize_t(_PyDict_SizeOf(mp));
Py_END_CRITICAL_SECTION();
return size;
}

static PyObject *
Expand All @@ -3711,24 +3735,31 @@ dict_or(PyObject *self, PyObject *other)
if (!PyDict_Check(self) || !PyDict_Check(other)) {
Py_RETURN_NOTIMPLEMENTED;
}
PyObject *new = PyDict_Copy(self);

PyObject *new = NULL;
Py_BEGIN_CRITICAL_SECTION2(self, other);
new = PyDict_Copy(self);
if (new == NULL) {
return NULL;
goto end;
}
if (dict_update_arg(new, other)) {
Py_DECREF(new);
return NULL;
Py_CLEAR(new);
goto end;
}

end:
Py_END_CRITICAL_SECTION2();
return new;
}

static PyObject *
dict_ior(PyObject *self, PyObject *other)
{
if (dict_update_arg(self, other)) {
return NULL;
}
return Py_NewRef(self);
int r;
Py_BEGIN_CRITICAL_SECTION2(self, other);
r = dict_update_arg(self, other);
Py_END_CRITICAL_SECTION2();
return r ? NULL : Py_NewRef(self);
}

PyDoc_STRVAR(getitem__doc__,
Expand Down Expand Up @@ -4610,14 +4641,15 @@ PyTypeObject PyDictRevIterKey_Type = {


/*[clinic input]
@critical_section
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want a critical section here. Creating the reverse iterator is thread-safe without any special handling, although iterating over the reverse iterator may require special handling.

dict.__reversed__

Return a reverse iterator over the dict keys.
[clinic start generated code]*/

static PyObject *
dict___reversed___impl(PyDictObject *self)
/*[clinic end generated code: output=e674483336d1ed51 input=23210ef3477d8c4d]*/
/*[clinic end generated code: output=e674483336d1ed51 input=6123e7c956063d86]*/
{
assert (PyDict_Check(self));
return dictiter_new(self, &PyDictRevIterKey_Type);
Expand Down Expand Up @@ -5246,7 +5278,11 @@ PyTypeObject PyDictKeys_Type = {
static PyObject *
dictkeys_new(PyObject *dict, PyObject *Py_UNUSED(ignored))
{
return _PyDictView_New(dict, &PyDictKeys_Type);
PyObject *view;
Py_BEGIN_CRITICAL_SECTION(dict);
view = _PyDictView_New(dict, &PyDictKeys_Type);
Copy link
Contributor

Choose a reason for hiding this comment

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

No critical section here. Creating the view objects, like _PyDictView_New (and the reverse iterator above) do not access any of the dictionary internals so they don't need to lock the dictionary.

Py_END_CRITICAL_SECTION();
return view;
}

static PyObject *
Expand Down Expand Up @@ -5348,7 +5384,11 @@ PyTypeObject PyDictItems_Type = {
static PyObject *
dictitems_new(PyObject *dict, PyObject *Py_UNUSED(ignored))
{
return _PyDictView_New(dict, &PyDictItems_Type);
PyObject *view;
Py_BEGIN_CRITICAL_SECTION(dict);
view = _PyDictView_New(dict, &PyDictItems_Type);
Copy link
Contributor

Choose a reason for hiding this comment

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

No critical section here.

Py_END_CRITICAL_SECTION();
return view;
}

static PyObject *
Expand Down Expand Up @@ -5429,7 +5469,11 @@ PyTypeObject PyDictValues_Type = {
static PyObject *
dictvalues_new(PyObject *dict, PyObject *Py_UNUSED(ignored))
{
return _PyDictView_New(dict, &PyDictValues_Type);
PyObject *view;
Py_BEGIN_CRITICAL_SECTION(dict);
view = _PyDictView_New(dict, &PyDictValues_Type);
Copy link
Contributor

Choose a reason for hiding this comment

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

No critical section here

Py_END_CRITICAL_SECTION();
return view;
}

static PyObject *
Expand Down