Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
2 changes: 2 additions & 0 deletions Include/internal/pycore_dict.h
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,8 @@ _PyInlineValuesSize(PyTypeObject *tp)
int
_PyDict_DetachFromObject(PyDictObject *dict, PyObject *obj);

PyDictObject *_PyObject_materialize_managed_dict_lock_held(PyObject *);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's name this like the other functions: _PyObject_MaterializeManagedDict_LockHeld and move the definition up next to _PyObject_MaterializeManagedDict.


#ifdef __cplusplus
}
#endif
Expand Down
8 changes: 0 additions & 8 deletions Include/object.h
Original file line number Diff line number Diff line change
Expand Up @@ -253,11 +253,7 @@ PyAPI_FUNC(PyTypeObject*) Py_TYPE(PyObject *ob);
#else
static inline PyTypeObject* _Py_TYPE(PyObject *ob)
{
#if defined(Py_GIL_DISABLED)
return (PyTypeObject *)_Py_atomic_load_ptr_relaxed(&ob->ob_type);
#else
return ob->ob_type;
#endif
}
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000
# define Py_TYPE(ob) _Py_TYPE(_PyObject_CAST(ob))
Expand Down Expand Up @@ -288,11 +284,7 @@ static inline int Py_IS_TYPE(PyObject *ob, PyTypeObject *type) {


static inline void Py_SET_TYPE(PyObject *ob, PyTypeObject *type) {
#ifdef Py_GIL_DISABLED
_Py_atomic_store_ptr(&ob->ob_type, type);
#else
ob->ob_type = type;
#endif
}
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000
# define Py_SET_TYPE(ob, type) Py_SET_TYPE(_PyObject_CAST(ob), type)
Expand Down
15 changes: 10 additions & 5 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,10 @@ ASSERT_DICT_LOCKED(PyObject *op)
if (!_PyInterpreterState_GET()->stoptheworld.world_stopped) { \
ASSERT_DICT_LOCKED(op); \
}
#define ASSERT_WORLD_STOPPED_OR_OBJ_LOCKED(op) \
if (!_PyInterpreterState_GET()->stoptheworld.world_stopped) { \
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op); \
}

#define IS_DICT_SHARED(mp) _PyObject_GC_IS_SHARED(mp)
#define SET_DICT_SHARED(mp) _PyObject_GC_SET_SHARED(mp)
Expand Down Expand Up @@ -227,6 +231,7 @@ static inline void split_keys_entry_added(PyDictKeysObject *keys)

#define ASSERT_DICT_LOCKED(op)
#define ASSERT_WORLD_STOPPED_OR_DICT_LOCKED(op)
#define ASSERT_WORLD_STOPPED_OR_OBJ_LOCKED(op)
#define LOCK_KEYS(keys)
#define UNLOCK_KEYS(keys)
#define ASSERT_KEYS_LOCKED(keys)
Expand Down Expand Up @@ -6703,10 +6708,10 @@ make_dict_from_instance_attributes(PyInterpreterState *interp,
return res;
}

static PyDictObject *
materialize_managed_dict_lock_held(PyObject *obj)
PyDictObject *
_PyObject_materialize_managed_dict_lock_held(PyObject *obj)
{
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(obj);
ASSERT_WORLD_STOPPED_OR_OBJ_LOCKED(obj);

PyDictValues *values = _PyObject_InlineValues(obj);
PyInterpreterState *interp = _PyInterpreterState_GET();
Expand Down Expand Up @@ -6735,7 +6740,7 @@ _PyObject_MaterializeManagedDict(PyObject *obj)
goto exit;
}
#endif
dict = materialize_managed_dict_lock_held(obj);
dict = _PyObject_materialize_managed_dict_lock_held(obj);

#ifdef Py_GIL_DISABLED
exit:
Expand Down Expand Up @@ -7170,7 +7175,7 @@ PyObject_ClearManagedDict(PyObject *obj)
int
_PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj)
{
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(obj);
ASSERT_WORLD_STOPPED_OR_OBJ_LOCKED(obj);
assert(_PyObject_ManagedDictPointer(obj)->dict == mp);
assert(_PyObject_InlineValuesConsistencyCheck(obj));

Expand Down
45 changes: 27 additions & 18 deletions Objects/typeobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -6626,6 +6626,12 @@ object_set_class(PyObject *self, PyObject *value, void *closure)
return -1;
}

#ifdef Py_GIL_DISABLED
PyInterpreterState *interp = _PyInterpreterState_GET();
// The real Py_TYPE(self) (`oldto`) may have changed from
// underneath us in another thread, so we stop the world.
_PyEval_StopTheWorld(interp);
#endif
PyTypeObject *oldto = Py_TYPE(self);

/* In versions of CPython prior to 3.5, the code in
Expand Down Expand Up @@ -6684,56 +6690,59 @@ object_set_class(PyObject *self, PyObject *value, void *closure)
PyErr_Format(PyExc_TypeError,
"__class__ assignment only supported for mutable types "
"or ModuleType subclasses");
return -1;
goto err;
}

if (compatible_for_assignment(oldto, newto, "__class__")) {
/* Changing the class will change the implicit dict keys,
* so we must materialize the dictionary first. */
if (oldto->tp_flags & Py_TPFLAGS_INLINE_VALUES) {
PyDictObject *dict = _PyObject_MaterializeManagedDict(self);
PyDictObject *dict = _PyObject_GetManagedDict(self);
if (dict == NULL) {
return -1;
dict = _PyObject_materialize_managed_dict_lock_held(self);
if (dict == NULL) {
goto err;
}
}

bool error = false;

Py_BEGIN_CRITICAL_SECTION2(self, dict);

// If we raced after materialization and replaced the dict
// then the materialized dict should no longer have the
// inline values in which case detach is a nop.
// Note: we don't need to lock here because the world should be stopped.

assert(_PyObject_GetManagedDict(self) == dict ||
dict->ma_values != _PyObject_InlineValues(self));

if (_PyDict_DetachFromObject(dict, self) < 0) {
error = true;
goto err;
}

Py_END_CRITICAL_SECTION2();
if (error) {
return -1;
}
}
if (newto->tp_flags & Py_TPFLAGS_HEAPTYPE) {
Py_INCREF(newto);
}
Py_BEGIN_CRITICAL_SECTION(self);
// The real Py_TYPE(self) (`oldto`) may have changed from
// underneath us in another thread, so we re-fetch it here.
oldto = Py_TYPE(self);

#ifdef Py_GIL_DISABLED
_PyEval_StartTheWorld(interp);
#endif
Py_SET_TYPE(self, newto);
Py_END_CRITICAL_SECTION();

if (oldto->tp_flags & Py_TPFLAGS_HEAPTYPE) {
Py_DECREF(oldto);
}

RARE_EVENT_INC(set_class);

return 0;
}
else {
return -1;
goto err;
}
err:
Copy link
Member

Choose a reason for hiding this comment

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

To have a more regular try: ... finally: _PyEval_StartTheWorld() pattern, you can add an int res = -1; variable, replace goto err with goto done, and set res to 0 on success (3 lines above).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @vstinner's suggestion is fine. Or you can refactor the parts that should be in a stop-the-world call into it's own function, like we often do for locks.

Another advantage of moving the body to a separate function is that it makes it more clear what data crosses the stop-the-world boundary -- some data loaded before the stop-the-world call may not be valid after it.

#ifdef Py_GIL_DISABLED
_PyEval_StartTheWorld(interp);
#endif
return -1;
}

static PyGetSetDef object_getsets[] = {
Expand Down