8000 Make PyDictValues thread safe · python/cpython@eadcc3e · GitHub
[go: up one dir, main page]
Skip to content

Commit eadcc3e

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

File tree

9 files changed

+184
-112
lines changed

9 files changed

+184
-112
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: 30 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,15 @@
11
import copy
2-
import pickle
32
import dis
3+
import pickle
44
import threading
55
import types
66
import unittest
7-
from test.support import threading_helper, check_impl_detail
7+
8+
from test.support import check_impl_detail, Py_GIL_DISABLED, threading_helper
89

910
# Skip this module on other interpreters, it is cpython specific:
1011
if check_impl_detail(cpython=False):
11-
raise unittest.SkipTest('implementation detail specific to cpython')
12+
raise unittest.SkipTest("implementation detail specific to cpython")
1213

1314
import _testinternalcapi
1415

@@ -28,6 +29,7 @@ def wrapper(*args, **kwargs):
2829
class TestLoadSuperAttrCache(unittest.TestCase):
2930
def test_descriptor_not_double_executed_on_spec_fail(self):
3031
calls = []
32+
3133
class Descriptor:
3234
def __get__(self, instance, owner):
3335
calls.append((instance, owner))
@@ -56,6 +58,7 @@ class Descriptor:
5658
class C:
5759
def __init__(self):
5860
self.x = 1
61+
5962
x = Descriptor()
6063

6164
def f(o):
@@ -921,9 +924,7 @@ def write(items):
921924
item.__globals__[None] = None
922925

923926
opname = "LOAD_GLOBAL_MODULE"
924-
self.assert_races_do_not_crash(
925-
opname, get_items, read, write, check_items=True
926-
)
927+
self.assert_races_do_not_crash(opname, get_items, read, write, check_items=True)
927928

928929
def test_store_attr_instance_value(self):
929930
def get_items():
@@ -1018,64 +1019,53 @@ def write(items):
10181019
opname = "UNPACK_SEQUENCE_LIST"
10191020
self.assert_races_do_not_crash(opname, get_items, read, write)
10201021

1022+
10211023
class C:
10221024
pass
10231025

1024-
class TestInstanceDict(unittest.TestCase):
10251026

1027+
class TestInstanceDict(unittest.TestCase):
10261028
def setUp(self):
10271029
c = C()
1028-
c.a, c.b, c.c = 0,0,0
1030+
c.a, c.b, c.c = 0, 0, 0
10291031

10301032
def test_values_on_instance(self):
10311033
c = C()
10321034
c.a = 1
10331035
C().b = 2
10341036
c.c = 3
1035-
self.assertEqual(
1036-
_testinternalcapi.get_object_dict_values(c),
1037-
(1, '<NULL>', 3)
1038-
)
1037+
self.assertEqual(_testinternalcapi.get_object_dict_values(c), (1, "<NULL>", 3))
10391038

10401039
def test_dict_materialization(self):
10411040
c = C()
10421041
c.a = 1
10431042
c.b = 2
10441043
c.__dict__
1045-
self.assertIs(
1046-
_testinternalcapi.get_object_dict_values(c),
1047-
None
1048-
)
1044+
self.assertIs(_testinternalcapi.get_object_dict_values(c), None)
10491045

1046+
@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
10501047
def test_dict_dematerialization(self):
10511048
c = C()
10521049
c.a = 1
10531050
c.b = 2
10541051
c.__dict__
1055-
self.assertIs(
1056-
_testinternalcapi.get_object_dict_values(c),
1057-
None
1058-
)
1052+
self.assertIs(_testinternalcapi.get_object_dict_values(c), None)
10591053
for _ in range(100):
10601054
c.a
1061-
self.assertEqual(
1062-
_testinternalcapi.get_object_dict_values(c),
1063-
(1, 2, '<NULL>')
1064-
)
1055+
self.assertEqual(_testinternalcapi.get_object_dict_values(c), (1, 2, "<NULL>"))
10651056

1057+
@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
10661058
def test_dict_dematerialization_multiple_refs(self):
10671059
c = C()
10681060
c.a = 1
10691061
c.b = 2
10701062
d = c.__dict__
10711063
for _ in range(100):
10721064
c.a
1073-
self.assertIs(
1074-
_testinternalcapi.get_object_dict_values(c),
1075-
None
1076-
)
1065+
self.assertIs(_testinternalcapi.get_object_dict_values(c), None)
10771066
self.assertIs(c.__dict__, d)
10781067

1068+
@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
10791069
def test_dict_dematerialization_copy(self):
10801070
c = C()
10811071
c.a = 1
@@ -1084,24 +1074,16 @@ def test_dict_dematerialization_copy(self):
10841074
for _ in range(100):
10851075
c.a
10861076
c2.a
1087-
self.assertEqual(
1088-
_testinternalcapi.get_object_dict_values(c),
1089-
(1, 2, '<NULL>')
1090-
)
1091-
self.assertEqual(
1092-
_testinternalcapi.get_object_dict_values(c2),
1093-
(1, 2, '<NULL>')
1094-
)
1077+
self.assertEqual(_testinternalcapi.get_object_dict_values(c), (1, 2, "<NULL>"))
1078+
self.assertEqual(_testinternalcapi.get_object_dict_values(c2), (1, 2, "<NULL>"))
10951079
c3 = copy.deepcopy(c)
10961080
for _ in range(100):
10971081
c.a
10981082
c3.a
1099-
self.assertEqual(
1100-
_testinternalcapi.get_object_dict_values(c),
1101-
(1, 2, '<NULL>')
1102-
)
1103-
#NOTE -- c3.__dict__ does not de-materialize
1083+
self.assertEqual(_testinternalcapi.get_object_dict_values(c), (1, 2, "<NULL>"))
1084+
# NOTE -- c3.__dict__ does not de-materialize
11041085

1086+
@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
11051087
def test_dict_dematerialization_pickle(self):
11061088
c = C()
11071089
c.a = 1
@@ -1110,31 +1092,21 @@ def test_dict_dematerialization_pickle(self):
11101092
for _ in range(100):
11111093
c.a
11121094
c2.a
1113-
self.assertEqual(
1114-
_testinternalcapi.get_object_dict_values(c),
1115-
(1, 2, '<NULL>')
1116-
)
1117-
self.assertEqual(
1118-
_testinternalcapi.get_object_dict_values(c2),
1119-
(1, 2, '<NULL>')
1120-
)
1095+
self.assertEqual(_testinternalcapi.get_object_dict_values(c), (1, 2, "<NULL>"))
1096+
self.assertEqual(_testinternalcapi.get_object_dict_values(c2), (1, 2, "<NULL>"))
11211097

11221098
def test_dict_dematerialization_subclass(self):
1123-
class D(dict): pass
1099+
class D(dict):
1100+
pass
1101+
11241102
c = C()
11251103
c.a = 1
11261104
c.b = 2
11271105
c.__dict__ = D(c.__dict__)
11281106
for _ in range(100):
11291107
c.a
1130-
self.assertIs(
1131-
_testinternalcapi.get_object_dict_values(c),
1132-
None
1133-
)
1134-
self.assertEqual(
1135-
c.__dict__,
1136-
{'a':1, 'b':2}
1137-
)
1108+
self.assertIs(_testinternalcapi.get_object_dict_values(c), None)
1109+
self.assertEqual(c.__dict__, {"a": 1, "b": 2})
11381110

11391111

11401112
if __name__ == "__main__":

0 commit comments

Comments
 (0)
0