Skip to content

Commit 3c3537e

Browse files
DinoVDinoV
committed
Make PyDictValues thread safe
1 parent fbb0169 commit 3c3537e

File tree

9 files changed

+159
-41
lines changed

9 files changed

+159
-41
lines changed

‎Include/internal/pycore_object.h‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ extern "C" {
1313
#include "pycore_emscripten_trampoline.h" // _PyCFunction_TrampolineCall()
1414
#include "pycore_interp.h" // PyInterpreterState.gc
1515
#include "pycore_pystate.h" // _PyInterpreterState_GET()
16+
#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION
1617

1718
/* Check if an object is consistent. For example, ensure that the reference
1819
counter is greater than or equal to 1, and ensure that ob_type is not NULL.
@@ -655,24 +656,73 @@ _PyDictOrValues_IsValues(PyDictOrValues dorv)
655656
return ((uintptr_t)dorv.values) & 1;
656657
}
657658

659+
// Should only be used when we know the object isn't racing with other
660+
// threads (e.g. finalization or GC).
658661
static inline PyDictValues *
659662
_PyDictOrValues_GetValues(PyDictOrValues dorv)
660663
{
661664
assert(_PyDictOrValues_IsValues(dorv));
662665
return (PyDictValues *)(dorv.values + 1);
663666
}
664667

668+
static inline PyDictValues *
669+
_PyDictOrValues_TryGetValues(PyObject *obj)
670+
{
671+
#ifdef Py_GIL_DISABLED
672+
if (_Py_IsOwnedByCurrentThread((PyObject *)obj) || _PyObject_GC_IS_SHARED(obj)) {
673+
PyDictOrValues *dorv = _PyObject_DictOrValuesPointer(obj);
674+
char *values = _Py_atomic_load_ptr_relaxed(&dorv->values);
675+
if (((uintptr_t)values) & 1) {
676+
return (PyDictValues *)(values + 1);
677+
}
678+
return NULL;
679+
}
680+
681+
// Make sure we don't race with materialization of the dict which will
682+
// propagate the shared bit down to the dict
683+
Py_BEGIN_CRITICAL_SECTION(obj);
684+
_PyObject_GC_SET_SHARED(obj);
685+
Py_END_CRITICAL_SECTION();
686+
687+
return _PyDictOrValues_TryGetValues(obj);
688+
#else
689+
char *values = _PyObject_DictOrValuesPointer(obj)->values;
690+
if (((uintptr_t)values) & 1) {
691+
return (PyDictValues *)(values + 1);
692+
}
693+
return NULL;
694+
#endif
695+
}
696+
665697
static inline PyObject *
666698
_PyDictOrValues_GetDict(PyDictOrValues dorv)
667699
{
668700
assert(!_PyDictOrValues_IsValues(dorv));
701+
#ifdef Py_GIL_DISABLED
702+
return _Py_atomic_load_ptr_relaxed(&dorv.dict);
703+
#else
669704
return dorv.dict;
705+
#endif
670706
}
671707

672708
static inline void
673709
_PyDictOrValues_SetValues(PyDictOrValues *ptr, PyDictValues *values)
674710
{
711+
#ifdef Py_GIL_DISABLED
712+
_Py_atomic_store_ptr_relaxed(&ptr->values, (char *)values - 1);
713+
#else
675714
ptr->values = ((char *)values) - 1;
715+
#endif
716+
}
717+
718+
static inline void
719+
_PyDictOrValues_SetDict(PyDictOrValues *ptr, PyObject *dict)
720+
{
721+
#ifdef Py_GIL_DISABLED
722+
_Py_atomic_store_ptr_relaxed(&ptr->dict, dict);
723+
#else
724+
ptr->dict = dict;
725+
#endif
676726
}
677727

678728
extern PyObject ** _PyObject_ComputedDictPointer(PyObject *);

‎Include/internal/pycore_opcode_metadata.h‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Include/internal/pycore_uop_metadata.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ const uint16_t _PyUop_Flags[MAX_UOP_ID+1] = {
114114
[_LOAD_ATTR] = HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG,
115115
[_GUARD_TYPE_VERSION] = HAS_DEOPT_FLAG | HAS_PASSTHROUGH_FLAG,
116116
[_CHECK_MANAGED_OBJECT_HAS_VALUES] = HAS_DEOPT_FLAG | HAS_PASSTHROUGH_FLAG,
117-
[_LOAD_ATTR_INSTANCE_VALUE] = HAS_ARG_FLAG | HAS_DEOPT_FLAG,
117+
[_LOAD_ATTR_INSTANCE_VALUE] = HAS_ARG_FLAG | HAS_DEOPT_FLAG | HAS_ESCAPES_FLAG,
118118
[_CHECK_ATTR_MODULE] = HAS_DEOPT_FLAG | HAS_PASSTHROUGH_FLAG,
119119
[_LOAD_ATTR_MODULE] = HAS_ARG_FLAG | HAS_DEOPT_FLAG,
120120
[_CHECK_ATTR_WITH_HINT] = HAS_DEOPT_FLAG | HAS_ESCAPES_FLAG | HAS_PASSTHROUGH_FLAG,
@@ -123,7 +123,7 @@ const uint16_t _PyUop_Flags[MAX_UOP_ID+1] = {
123123
[_CHECK_ATTR_CLASS] = HAS_DEOPT_FLAG | HAS_PASSTHROUGH_FLAG,
124124
[_LOAD_ATTR_CLASS] = HAS_ARG_FLAG,
125125
[_GUARD_DORV_VALUES] = HAS_DEOPT_FLAG | HAS_PASSTHROUGH_FLAG,
126-
[_STORE_ATTR_INSTANCE_VALUE] = HAS_ESCAPES_FLAG,
126+
[_STORE_ATTR_INSTANCE_VALUE] = HAS_DEOPT_FLAG | HAS_ESCAPES_FLAG,
127127
[_STORE_ATTR_SLOT] = HAS_ESCAPES_FLAG,
128128
[_COMPARE_OP] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG,
129129
[_COMPARE_OP_FLOAT] = HAS_ARG_FLAG | HAS_DEOPT_FLAG | HAS_ESCAPES_FLAG,

‎Lib/test/test_opcache.py‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
import threading
55
import types
66
import unittest
7-
from test.support import threading_helper, check_impl_detail
7+
from test.support import threading_helper, check_impl_detail, Py_GIL_DISABLED
88

99
# Skip this module on other interpreters, it is cpython specific:
1010
if check_impl_detail(cpython=False):
@@ -1047,6 +1047,7 @@ def test_dict_materialization(self):
10471047
None
10481048
)
10491049

1050+
@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
10501051
def test_dict_dematerialization(self):
10511052
c = C()
10521053
c.a = 1
@@ -1063,6 +1064,7 @@ def test_dict_dematerialization(self):
10631064
(1, 2, '<NULL>')
10641065
)
10651066

1067+
@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
10661068
def test_dict_dematerialization_multiple_refs(self):
10671069
c = C()
10681070
c.a = 1
@@ -1076,6 +1078,7 @@ def test_dict_dematerialization_multiple_refs(self):
10761078
)
10771079
self.assertIs(c.__dict__, d)
10781080

1081+
@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
10791082
def test_dict_dematerialization_copy(self):
10801083
c = C()
10811084
c.a = 1
@@ -1102,6 +1105,7 @@ def test_dict_dematerialization_copy(self):
11021105
)
11031106
#NOTE -- c3.__dict__ does not de-materialize
11041107

1108+
@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
11051109
def test_dict_dematerialization_pickle(self):
11061110
c = C()
11071111
c.a = 1
@@ -1119,6 +1123,7 @@ def test_dict_dematerialization_pickle(self):
11191123
(1, 2, '<NULL>')
11201124
)
11211125

1126+
@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
11221127
def test_dict_dematerialization_subclass(self):
11231128
class D(dict): pass
11241129
c = C()

‎Objects/dictobject.c‎

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6005,6 +6005,10 @@ has_unique_reference(PyObject *op)
60056005
bool
60066006
_PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv)
60076007
{
6008+
#ifdef Py_GIL_DISABLED
6009+
// We don't yet support dematerialization in no-GIL builds
6010+
return false;
6011+
#else
60086012
assert(_PyObject_DictOrValuesPointer(obj) == dorv);
60096013
assert(!_PyDictOrValues_IsValues(*dorv));
60106014
PyDictObject *dict = (PyDictObject *)_PyDictOrValues_GetDict(*dorv);
@@ -6032,6 +6036,7 @@ _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv)
60326036
dict->ma_values = NULL;
60336037
Py_DECREF(dict);
60346038
return true;
6039+
#endif
60356040
}
60366041

60376042
int
@@ -6066,7 +6071,7 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values,
60666071
if (dict == NULL) {
60676072
return -1;
60686073
}
6069-
_PyObject_DictOrValuesPointer(obj)->dict = dict;
6074+
_PyDictOrValues_SetDict(_PyObject_DictOrValuesPointer(obj), dict);
60706075
if (value == NULL) {
60716076
return PyDict_DelItem(dict, name);
60726077
}
@@ -6150,10 +6155,11 @@ _PyObject_IsInstanceDictEmpty(PyObject *obj)
61506155
PyObject *dict;
61516156
if (tp->tp_flags & Py_TPFLAGS_MANAGED_DICT) {
61526157
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(obj);
6153-
if (_PyDictOrValues_IsValues(dorv)) {
6158+
PyDictValues *values;
6159+
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
61546160
PyDictKeysObject *keys = CACHED_KEYS(tp);
61556161
for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) {
6156-
if (_PyDictOrValues_GetValues(dorv)->values[i] != NULL) {
6162+
if (values->values[i] != NULL) {
61576163
return 0;
61586164
}
61596165
}
@@ -6176,15 +6182,15 @@ _PyObject_FreeInstanceAttributes(PyObject *self)
61766182
{
61776183
PyTypeObject *tp = Py_TYPE(self);
61786184
assert(Py_TYPE(self)->tp_flags & Py_TPFLAGS_MANAGED_DICT);
6179-
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(self);
6180-
if (!_PyDictOrValues_IsValues(dorv)) {
6185+
PyDictValues *values = _PyDictOrValues_TryGetValues(self);
6186+
if (values == NULL) {
61816187
return;
61826188
}
6183-
PyDictValues *values = _PyDictOrValues_GetValues(dorv);
61846189
PyDictKeysObject *keys = CACHED_KEYS(tp);
61856190
for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) {
61866191
Py_XDECREF(values->values[i]);
61876192
}
6193+
// TODO: Free with qsbr if we're shared
61886194
free_values(values);
61896195
}
61906196

@@ -6195,6 +6201,9 @@ PyObject_VisitManagedDict(PyObject *obj, visitproc visit, void *arg)
61956201
if((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT) == 0) {
61966202
return 0;
61976203
}
6204+
#ifdef Py_GIL_DISABLED
6205+
assert(_PyInterpreterState_GET()->stoptheworld.world_stopped);
6206+
#endif
61986207
assert(tp->tp_dictoffset);
61996208
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(obj);
62006209
if (_PyDictOrValues_IsValues(dorv)) {
@@ -6225,13 +6234,14 @@ PyObject_ClearManagedDict(PyObject *obj)
62256234
for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) {
62266235
Py_CLEAR(values->values[i]);
62276236
}
6228-
dorv_ptr->dict = NULL;
6237+
_PyDictOrValues_SetDict(dorv_ptr, NULL);
6238+
// TODO: Free with qsbr
62296239
free_values(values);
62306240
}
62316241
else {
6232-
PyObject *dict = dorv_ptr->dict;
6242+
PyObject *dict = _PyDictOrValues_GetDict(*dorv_ptr);
62336243
if (dict) {
6234-
dorv_ptr->dict = NULL;
6244+
_PyDictOrValues_SetDict(dorv_ptr, NULL);
62356245
Py_DECREF(dict);
62366246
}
62376247
}
@@ -6245,22 +6255,32 @@ PyObject_GenericGetDict(PyObject *obj, void *context)
62456255
PyTypeObject *tp = Py_TYPE(obj);
62466256
if (_PyType_HasFeature(tp, Py_TPFLAGS_MANAGED_DICT)) {
62476257
PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj);
6248-
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
6249-
PyDictValues *values = _PyDictOrValues_GetValues(*dorv_ptr);
6258+
PyDictValues *values;
6259+
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
62506260
OBJECT_STAT_INC(dict_materialized_on_request);
6261+
Py_BEGIN_CRITICAL_SECTION(obj);
62516262
dict = make_dict_from_instance_attributes(
62526263
interp, CACHED_KEYS(tp), values);
62536264
if (dict != NULL) {
6254-
dorv_ptr->dict = dict;
6265+
#ifdef Py_GIL_DISABLED
6266+
if (_PyObject_GC_IS_SHARED(obj)) {
6267+
// The values were accessed from multiple threads and need to be
6268+
// freed via QSBR, now the dict needs to be shared as it owns the
6269+
// values.
6270+
_PyObject_GC_SET_SHARED(dict);
6271+
}
6272+
#endif
6273+
_PyDictOrValues_SetDict(dorv_ptr, dict);
62556274
}
6275+
Py_END_CRITICAL_SECTION();
62566276
}
62576277
else {
62586278
dict = _PyDictOrValues_GetDict(*dorv_ptr);
62596279
if (dict == NULL) {
62606280
dictkeys_incref(CACHED_KEYS(tp));
62616281
OBJECT_STAT_INC(dict_materialized_on_request);
62626282
dict = new_dict_with_shared_keys(interp, CACHED_KEYS(tp));
6263-
dorv_ptr->dict = dict;
6283+
_PyDictOrValues_SetDict(dorv_ptr, dict);
62646284
}
62656285
}
62666286
}

‎Objects/object.c‎

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1398,13 +1398,38 @@ _PyObject_GetDictPtr(PyObject *obj)
13981398
return _PyObject_ComputedDictPointer(obj);
13991399
}
14001400
PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj);
1401-
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
1402-
PyObject *dict = _PyObject_MakeDictFromInstanceAttributes(obj, _PyDictOrValues_GetValues(*dorv_ptr));
1401+
PyDictValues *values;
1402+
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
1403+
PyObject *dict;
1404+
Py_BEGIN_CRITICAL_SECTION(obj);
1405+
#ifdef Py_GIL_DISABLED
1406+
if ((values = _PyDictOrValues_TryGetValues(obj)) == NULL) {
1407+
dict = _PyDictOrValues_GetDict(*dorv_ptr);
1408+
goto done;
1409+
}
1410+
#endif
1411+
1412+
dict = _PyObject_MakeDictFromInstanceAttributes(obj, values);
1413+
if (dict == NULL) {
1414+
goto done;
1415+
}
1416+
1417+
#ifdef Py_GIL_DISABLED
1418+
if (_PyObject_GC_IS_SHARED(obj)) {
1419+
// The values were accessed from multiple threads and need to be
1420+
// freed via QSBR, now the dict needs to be shared as it owns the
1421+
// values.
1422+
_PyObject_GC_SET_SHARED(dict);
1423+
}
1424+
#endif
1425+
dorv_ptr->dict = dict;
1426+
1427+
done:
1428+
Py_END_CRITICAL_SECTION();
14031429
if (dict == NULL) {
14041430
PyErr_Clear();
14051431
return NULL;
14061432
}
1407-
dorv_ptr->dict = dict;
14081433
}
14091434
return &dorv_ptr->dict;
14101435
}
@@ -1477,8 +1502,8 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method)
14771502
PyObject *dict;
14781503
if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) {
14791504
PyDictOrValues* dorv_ptr = _PyObject_DictOrValuesPointer(obj);
1480-
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
1481-
PyDictValues *values = _PyDictOrValues_GetValues(*dorv_ptr);
1505+
PyDictValues *values;
1506+
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
14821507
PyObject *attr = _PyObject_GetInstanceAttribute(obj, values, name);
14831508
if (attr != NULL) {
14841509
*method = attr;
@@ -1584,8 +1609,8 @@ _PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name,
15841609
if (dict == NULL) {
15851610
if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) {
15861611
PyDictOrValues* dorv_ptr = _PyObject_DictOrValuesPointer(obj);
1587-
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
1588-
PyDictValues *values = _PyDictOrValues_GetValues(*dorv_ptr);
1612+
PyDictValues *values;
1613+
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
15891614
if (PyUnicode_CheckExact(name)) {
15901615
res = _PyObject_GetInstanceAttribute(obj, values, name);
15911616
if (res != NULL) {
@@ -1700,19 +1725,22 @@ _PyObject_GenericSetAttrWithDict(PyObject *obj, PyObject *name,
17001725
PyObject **dictptr;
17011726
if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) {
17021727
PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj);
1703-
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
1728+
PyDictValues *values;
1729+
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
17041730
res = _PyObject_StoreInstanceAttribute(
1705-
obj, _PyDictOrValues_GetValues(*dorv_ptr), name, value);
1731+
obj, values, name, value);
17061732
goto error_check;
17071733
}
17081734
dictptr = &dorv_ptr->dict;
17091735
if (*dictptr == NULL) {
17101736
if (_PyObject_InitInlineValues(obj, tp) < 0) {
17111737
goto done;
17121738
}
1713-
res = _PyObject_StoreInstanceAttribute(
1714-
obj, _PyDictOrValues_GetValues(*dorv_ptr), name, value);
1715-
goto error_check;
1739+
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
1740+
res = _PyObject_StoreInstanceAttribute(
1741+
obj, values, name, value);
1742+
goto error_check;
1743+
}
17161744
}
17171745
}
17181746
else {

0 commit comments

Comments
 (0)