Skip to content

Commit c694324

Browse files
DinoVDinoV
committed
Make PyDictValues thread safe
1 parent 347acde commit c694324

File tree

9 files changed

+164
-45
lines changed

9 files changed

+164
-45
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: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎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: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6439,6 +6439,10 @@ has_unique_reference(PyObject *op)
64396439
bool
64406440
_PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv)
64416441
{
6442+
#ifdef Py_GIL_DISABLED
6443+
// We don't yet support dematerialization in no-GIL builds
6444+
return false;
6445+
#else
64426446
assert(_PyObject_DictOrValuesPointer(obj) == dorv);
64436447
assert(!_PyDictOrValues_IsValues(*dorv));
64446448
PyDictObject *dict = (PyDictObject *)_PyDictOrValues_GetDict(*dorv);
@@ -6468,6 +6472,7 @@ _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv)
64686472
dict->ma_values = NULL;
64696473
Py_DECREF(dict);
64706474
return true;
6475+
#endif
64716476
}
64726477

64736478
int
@@ -6506,7 +6511,7 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values,
65066511
if (dict == NULL) {
65076512
return -1;
65086513
}
6509-
_PyObject_DictOrValuesPointer(obj)->dict = dict;
6514+
_PyDictOrValues_SetDict(_PyObject_DictOrValuesPointer(obj), dict);
65106515
if (value == NULL) {
65116516
return PyDict_DelItem(dict, name);
65126517
}
@@ -6590,10 +6595,11 @@ _PyObject_IsInstanceDictEmpty(PyObject *obj)
65906595
PyObject *dict;
65916596
if (tp->tp_flags & Py_TPFLAGS_MANAGED_DICT) {
65926597
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(obj);
6593-
if (_PyDictOrValues_IsValues(dorv)) {
6598+
PyDictValues *values;
6599+
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
65946600
PyDictKeysObject *keys = CACHED_KEYS(tp);
65956601
for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) {
6596-
if (_PyDictOrValues_GetValues(dorv)->values[i] != NULL) {
6602+
if (values->values[i] != NULL) {
65976603
return 0;
65986604
}
65996605
}
@@ -6616,11 +6622,10 @@ _PyObject_FreeInstanceAttributes(PyObject *self)
66166622
{
66176623
PyTypeObject *tp = Py_TYPE(self);
66186624
assert(Py_TYPE(self)->tp_flags & Py_TPFLAGS_MANAGED_DICT);
6619-
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(self);
6620-
if (!_PyDictOrValues_IsValues(dorv)) {
6625+
PyDictValues *values = _PyDictOrValues_TryGetValues(self);
6626+
if (values == NULL) {
66216627
return;
66226628
}
6623-
PyDictValues *values = _PyDictOrValues_GetValues(dorv);
66246629
PyDictKeysObject *keys = CACHED_KEYS(tp);
66256630
for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) {
66266631
Py_XDECREF(values->values[i]);
@@ -6635,6 +6640,9 @@ PyObject_VisitManagedDict(PyObject *obj, visitproc visit, void *arg)
66356640
if((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT) == 0) {
66366641
return 0;
66376642
}
6643+
#ifdef Py_GIL_DISABLED
6644+
assert(_PyInterpreterState_GET()->stoptheworld.world_stopped);
6645+
#endif
66386646
assert(tp->tp_dictoffset);
66396647
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(obj);
66406648
if (_PyDictOrValues_IsValues(dorv)) {
@@ -6665,13 +6673,13 @@ PyObject_ClearManagedDict(PyObject *obj)
66656673
for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) {
66666674
Py_CLEAR(values->values[i]);
66676675
}
6668-
dorv_ptr->dict = NULL;
6676+
_PyDictOrValues_SetDict(dorv_ptr, NULL);
66696677
free_values(values, IS_DICT_SHARED((PyDictObject*)obj));
66706678
}
66716679
else {
6672-
PyObject *dict = dorv_ptr->dict;
6680+
PyObject *dict = _PyDictOrValues_GetDict(*dorv_ptr);
66736681
if (dict) {
6674-
dorv_ptr->dict = NULL;
6682+
_PyDictOrValues_SetDict(dorv_ptr, NULL);
66756683
Py_DECREF(dict);
66766684
}
66776685
}
@@ -6685,22 +6693,32 @@ PyObject_GenericGetDict(PyObject *obj, void *context)
66856693
PyTypeObject *tp = Py_TYPE(obj);
66866694
if (_PyType_HasFeature(tp, Py_TPFLAGS_MANAGED_DICT)) {
66876695
PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj);
6688-
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
6689-
PyDictValues *values = _PyDictOrValues_GetValues(*dorv_ptr);
6696+
PyDictValues *values;
6697+
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
66906698
OBJECT_STAT_INC(dict_materialized_on_request);
6699+
Py_BEGIN_CRITICAL_SECTION(obj);
66916700
dict = make_dict_from_instance_attributes(
66926701
interp, CACHED_KEYS(tp), values);
66936702
if (dict != NULL) {
6694-
dorv_ptr->dict = dict;
6703+
#ifdef Py_GIL_DISABLED
6704+
if (_PyObject_GC_IS_SHARED(obj)) {
6705+
// The values were accessed from multiple threads and need to be
6706+
// freed via QSBR, now the dict needs to be shared as it owns the
6707+
// values.
6708+
_PyObject_GC_SET_SHARED(dict);
6709+
}
6710+
#endif
6711+
_PyDictOrValues_SetDict(dorv_ptr, dict);
66956712
}
6713+
Py_END_CRITICAL_SECTION();
66966714
}
66976715
else {
66986716
dict = _PyDictOrValues_GetDict(*dorv_ptr);
66996717
if (dict == NULL) {
67006718
dictkeys_incref(CACHED_KEYS(tp));
67016719
OBJECT_STAT_INC(dict_materialized_on_request);
67026720
dict = new_dict_with_shared_keys(interp, CACHED_KEYS(tp));
6703-
dorv_ptr->dict = dict;
6721+
_PyDictOrValues_SetDict(dorv_ptr, dict);
67046722
}
67056723
}
67066724
}

‎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)