Skip to content

Commit

Permalink
Make PyDictValues thread safe
Browse files Browse the repository at this point in the history
  • Loading branch information
@DinoV
DinoV committed Feb 16, 2024
1 parent fbb0169 commit 3c3537e
Show file tree
Hide file tree
Showing 9 changed files with 159 additions and 41 deletions.
50 changes: 50 additions & 0 deletions Include/internal/pycore_object.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ extern "C" {
#include "pycore_emscripten_trampoline.h" // _PyCFunction_TrampolineCall()
#include "pycore_interp.h" // PyInterpreterState.gc
#include "pycore_pystate.h" // _PyInterpreterState_GET()
#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION

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

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

static inline PyDictValues *
_PyDictOrValues_TryGetValues(PyObject *obj)
{
#ifdef Py_GIL_DISABLED
if (_Py_IsOwnedByCurrentThread((PyObject *)obj) || _PyObject_GC_IS_SHARED(obj)) {
PyDictOrValues *dorv = _PyObject_DictOrValuesPointer(obj);
char *values = _Py_atomic_load_ptr_relaxed(&dorv->values);
if (((uintptr_t)values) & 1) {
return (PyDictValues *)(values + 1);
}
return NULL;
}

// Make sure we don't race with materialization of the dict which will
// propagate the shared bit down to the dict
Py_BEGIN_CRITICAL_SECTION(obj);
_PyObject_GC_SET_SHARED(obj);
Py_END_CRITICAL_SECTION();

return _PyDictOrValues_TryGetValues(obj);
#else
char *values = _PyObject_DictOrValuesPointer(obj)->values;
if (((uintptr_t)values) & 1) {
return (PyDictValues *)(values + 1);
}
return NULL;
#endif
}

static inline PyObject *
_PyDictOrValues_GetDict(PyDictOrValues dorv)
{
assert(!_PyDictOrValues_IsValues(dorv));
#ifdef Py_GIL_DISABLED
return _Py_atomic_load_ptr_relaxed(&dorv.dict);
#else
return dorv.dict;
#endif
}

static inline void
_PyDictOrValues_SetValues(PyDictOrValues *ptr, PyDictValues *values)
{
#ifdef Py_GIL_DISABLED
_Py_atomic_store_ptr_relaxed(&ptr->values, (char *)values - 1);
#else
ptr->values = ((char *)values) - 1;
#endif
}

static inline void
_PyDictOrValues_SetDict(PyDictOrValues *ptr, PyObject *dict)
{
#ifdef Py_GIL_DISABLED
_Py_atomic_store_ptr_relaxed(&ptr->dict, dict);
#else
ptr->dict = dict;
#endif
}

extern PyObject ** _PyObject_ComputedDictPointer(PyObject *);
Expand Down
2 changes: 1 addition & 1 deletion Include/internal/pycore_opcode_metadata.h
View file

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

4 changes: 2 additions & 2 deletions Include/internal/pycore_uop_metadata.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ const uint16_t _PyUop_Flags[MAX_UOP_ID+1] = {
[_LOAD_ATTR] = HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG,
[_GUARD_TYPE_VERSION] = HAS_DEOPT_FLAG | HAS_PASSTHROUGH_FLAG,
[_CHECK_MANAGED_OBJECT_HAS_VALUES] = HAS_DEOPT_FLAG | HAS_PASSTHROUGH_FLAG,
[_LOAD_ATTR_INSTANCE_VALUE] = HAS_ARG_FLAG | HAS_DEOPT_FLAG,
[_LOAD_ATTR_INSTANCE_VALUE] = HAS_ARG_FLAG | HAS_DEOPT_FLAG | HAS_ESCAPES_FLAG,
[_CHECK_ATTR_MODULE] = HAS_DEOPT_FLAG | HAS_PASSTHROUGH_FLAG,
[_LOAD_ATTR_MODULE] = HAS_ARG_FLAG | HAS_DEOPT_FLAG,
[_CHECK_ATTR_WITH_HINT] = HAS_DEOPT_FLAG | HAS_ESCAPES_FLAG | HAS_PASSTHROUGH_FLAG,
Expand All @@ -123,7 +123,7 @@ const uint16_t _PyUop_Flags[MAX_UOP_ID+1] = {
[_CHECK_ATTR_CLASS] = HAS_DEOPT_FLAG | HAS_PASSTHROUGH_FLAG,
[_LOAD_ATTR_CLASS] = HAS_ARG_FLAG,
[_GUARD_DORV_VALUES] = HAS_DEOPT_FLAG | HAS_PASSTHROUGH_FLAG,
[_STORE_ATTR_INSTANCE_VALUE] = HAS_ESCAPES_FLAG,
[_STORE_ATTR_INSTANCE_VALUE] = HAS_DEOPT_FLAG | HAS_ESCAPES_FLAG,
[_STORE_ATTR_SLOT] = HAS_ESCAPES_FLAG,
[_COMPARE_OP] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG,
[_COMPARE_OP_FLOAT] = HAS_ARG_FLAG | HAS_DEOPT_FLAG | HAS_ESCAPES_FLAG,
Expand Down
7 changes: 6 additions & 1 deletion Lib/test/test_opcache.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import threading
import types
import unittest
from test.support import threading_helper, check_impl_detail
from test.support import threading_helper, check_impl_detail, Py_GIL_DISABLED

# Skip this module on other interpreters, it is cpython specific:
if check_impl_detail(cpython=False):
Expand Down Expand Up @@ -1047,6 +1047,7 @@ def test_dict_materialization(self):
None
)

@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
def test_dict_dematerialization(self):
c = C()
c.a = 1
Expand All @@ -1063,6 +1064,7 @@ def test_dict_dematerialization(self):
(1, 2, '<NULL>')
)

@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
def test_dict_dematerialization_multiple_refs(self):
c = C()
c.a = 1
Expand All @@ -1076,6 +1078,7 @@ def test_dict_dematerialization_multiple_refs(self):
)
self.assertIs(c.__dict__, d)

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

@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
def test_dict_dematerialization_pickle(self):
c = C()
c.a = 1
Expand All @@ -1119,6 +1123,7 @@ def test_dict_dematerialization_pickle(self):
(1, 2, '<NULL>')
)

@unittest.skipIf(Py_GIL_DISABLED, "dematerialization disabled without the GIL")
def test_dict_dematerialization_subclass(self):
class D(dict): pass
c = C()
Expand Down
46 changes: 33 additions & 13 deletions Objects/dictobject.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6005,6 +6005,10 @@ has_unique_reference(PyObject *op)
bool
_PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv)
{
#ifdef Py_GIL_DISABLED
// We don't yet support dematerialization in no-GIL builds
return false;
#else
assert(_PyObject_DictOrValuesPointer(obj) == dorv);
assert(!_PyDictOrValues_IsValues(*dorv));
PyDictObject *dict = (PyDictObject *)_PyDictOrValues_GetDict(*dorv);
Expand Down Expand Up @@ -6032,6 +6036,7 @@ _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv)
dict->ma_values = NULL;
Py_DECREF(dict);
return true;
#endif
}

int
Expand Down Expand Up @@ -6066,7 +6071,7 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values,
if (dict == NULL) {
return -1;
}
_PyObject_DictOrValuesPointer(obj)->dict = dict;
_PyDictOrValues_SetDict(_PyObject_DictOrValuesPointer(obj), dict);
if (value == NULL) {
return PyDict_DelItem(dict, name);
}
Expand Down Expand Up @@ -6150,10 +6155,11 @@ _PyObject_IsInstanceDictEmpty(PyObject *obj)
PyObject *dict;
if (tp->tp_flags & Py_TPFLAGS_MANAGED_DICT) {
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(obj);
if (_PyDictOrValues_IsValues(dorv)) {
PyDictValues *values;
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
PyDictKeysObject *keys = CACHED_KEYS(tp);
for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) {
if (_PyDictOrValues_GetValues(dorv)->values[i] != NULL) {
if (values->values[i] != NULL) {
return 0;
}
}
Expand All @@ -6176,15 +6182,15 @@ _PyObject_FreeInstanceAttributes(PyObject *self)
{
PyTypeObject *tp = Py_TYPE(self);
assert(Py_TYPE(self)->tp_flags & Py_TPFLAGS_MANAGED_DICT);
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(self);
if (!_PyDictOrValues_IsValues(dorv)) {
PyDictValues *values = _PyDictOrValues_TryGetValues(self);
if (values == NULL) {
return;
}
PyDictValues *values = _PyDictOrValues_GetValues(dorv);
PyDictKeysObject *keys = CACHED_KEYS(tp);
for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) {
Py_XDECREF(values->values[i]);
}
// TODO: Free with qsbr if we're shared
free_values(values);
}

Expand All @@ -6195,6 +6201,9 @@ PyObject_VisitManagedDict(PyObject *obj, visitproc visit, void *arg)
if((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT) == 0) {
return 0;
}
#ifdef Py_GIL_DISABLED
assert(_PyInterpreterState_GET()->stoptheworld.world_stopped);
#endif
assert(tp->tp_dictoffset);
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(obj);
if (_PyDictOrValues_IsValues(dorv)) {
Expand Down Expand Up @@ -6225,13 +6234,14 @@ PyObject_ClearManagedDict(PyObject *obj)
for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) {
Py_CLEAR(values->values[i]);
}
dorv_ptr->dict = NULL;
_PyDictOrValues_SetDict(dorv_ptr, NULL);
// TODO: Free with qsbr
free_values(values);
}
else {
PyObject *dict = dorv_ptr->dict;
PyObject *dict = _PyDictOrValues_GetDict(*dorv_ptr);
if (dict) {
dorv_ptr->dict = NULL;
_PyDictOrValues_SetDict(dorv_ptr, NULL);
Py_DECREF(dict);
}
}
Expand All @@ -6245,22 +6255,32 @@ PyObject_GenericGetDict(PyObject *obj, void *context)
PyTypeObject *tp = Py_TYPE(obj);
if (_PyType_HasFeature(tp, Py_TPFLAGS_MANAGED_DICT)) {
PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj);
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
PyDictValues *values = _PyDictOrValues_GetValues(*dorv_ptr);
PyDictValues *values;
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
OBJECT_STAT_INC(dict_materialized_on_request);
Py_BEGIN_CRITICAL_SECTION(obj);
dict = make_dict_from_instance_attributes(
interp, CACHED_KEYS(tp), values);
if (dict != NULL) {
dorv_ptr->dict = dict;
#ifdef Py_GIL_DISABLED
if (_PyObject_GC_IS_SHARED(obj)) {
// The values were accessed from multiple threads and need to be
// freed via QSBR, now the dict needs to be shared as it owns the
// values.
_PyObject_GC_SET_SHARED(dict);
}
#endif
_PyDictOrValues_SetDict(dorv_ptr, dict);
}
Py_END_CRITICAL_SECTION();
}
else {
dict = _PyDictOrValues_GetDict(*dorv_ptr);
if (dict == NULL) {
dictkeys_incref(CACHED_KEYS(tp));
OBJECT_STAT_INC(dict_materialized_on_request);
dict = new_dict_with_shared_keys(interp, CACHED_KEYS(tp));
dorv_ptr->dict = dict;
_PyDictOrValues_SetDict(dorv_ptr, dict);
}
}
}
Expand Down
52 changes: 40 additions & 12 deletions Objects/object.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1398,13 +1398,38 @@ _PyObject_GetDictPtr(PyObject *obj)
return _PyObject_ComputedDictPointer(obj);
}
PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj);
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
PyObject *dict = _PyObject_MakeDictFromInstanceAttributes(obj, _PyDictOrValues_GetValues(*dorv_ptr));
PyDictValues *values;
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
PyObject *dict;
Py_BEGIN_CRITICAL_SECTION(obj);
#ifdef Py_GIL_DISABLED
if ((values = _PyDictOrValues_TryGetValues(obj)) == NULL) {
dict = _PyDictOrValues_GetDict(*dorv_ptr);
goto done;
}
#endif

dict = _PyObject_MakeDictFromInstanceAttributes(obj, values);
if (dict == NULL) {
goto done;
}

#ifdef Py_GIL_DISABLED
if (_PyObject_GC_IS_SHARED(obj)) {
// The values were accessed from multiple threads and need to be
// freed via QSBR, now the dict needs to be shared as it owns the
// values.
_PyObject_GC_SET_SHARED(dict);
}
#endif
dorv_ptr->dict = dict;

done:
Py_END_CRITICAL_SECTION();
if (dict == NULL) {
PyErr_Clear();
return NULL;
}
dorv_ptr->dict = dict;
}
return &dorv_ptr->dict;
}
Expand Down Expand Up @@ -1477,8 +1502,8 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method)
PyObject *dict;
if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) {
PyDictOrValues* dorv_ptr = _PyObject_DictOrValuesPointer(obj);
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
PyDictValues *values = _PyDictOrValues_GetValues(*dorv_ptr);
PyDictValues *values;
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
PyObject *attr = _PyObject_GetInstanceAttribute(obj, values, name);
if (attr != NULL) {
*method = attr;
Expand Down Expand Up @@ -1584,8 +1609,8 @@ _PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name,
if (dict == NULL) {
if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) {
PyDictOrValues* dorv_ptr = _PyObject_DictOrValuesPointer(obj);
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
PyDictValues *values = _PyDictOrValues_GetValues(*dorv_ptr);
PyDictValues *values;
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
if (PyUnicode_CheckExact(name)) {
res = _PyObject_GetInstanceAttribute(obj, values, name);
if (res != NULL) {
Expand Down Expand Up @@ -1700,19 +1725,22 @@ _PyObject_GenericSetAttrWithDict(PyObject *obj, PyObject *name,
PyObject **dictptr;
if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) {
PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj);
if (_PyDictOrValues_IsValues(*dorv_ptr)) {
PyDictValues *values;
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
res = _PyObject_StoreInstanceAttribute(
obj, _PyDictOrValues_GetValues(*dorv_ptr), name, value);
obj, values, name, value);
goto error_check;
}
dictptr = &dorv_ptr->dict;
if (*dictptr == NULL) {
if (_PyObject_InitInlineValues(obj, tp) < 0) {
goto done;
}
res = _PyObject_StoreInstanceAttribute(
obj, _PyDictOrValues_GetValues(*dorv_ptr), name, value);
goto error_check;
if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) {
res = _PyObject_StoreInstanceAttribute(
obj, values, name, value);
goto error_check;
}
}
}
else {
Expand Down

0 comments on commit 3c3537e

Please sign in to comment.