Skip to content

Commit 319f632

Browse files
committed
Refactor del/set into set_or_del_lock_held, and avoid locking on newly materialized dict
Fix duplicate incref Fix comment Remove redundant if check on detach
1 parent 91e82d6 commit 319f632

File tree

2 files changed

+37
-42
lines changed

2 files changed

+37
-42
lines changed

Include/internal/pycore_object.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -661,7 +661,7 @@ extern PyObject* _PyType_GetTextSignatureFromInternalDoc(const char *, const cha
661661

662662
void _PyObject_InitInlineValues(PyObject *obj, PyTypeObject *tp);
663663
extern int _PyObject_StoreInstanceAttribute(PyObject *obj,
664-
PyObject *name, PyObject *value);
664+
PyObject *name, PyObject *value);
665665
extern bool _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name,
666666
PyObject **attr);
667667

Objects/dictobject.c

Lines changed: 36 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -6659,6 +6659,22 @@ _PyObject_MaterializeManagedDict(PyObject *obj)
66596659
return dict;
66606660
}
66616661

6662+
static int
6663+
set_or_del_lock_held(PyDictObject *dict, PyObject *name, PyObject *value)
6664+
{
6665+
if (value == NULL) {
6666+
Py_hash_t hash;
6667+
if (!PyUnicode_CheckExact(name) || (hash = unicode_get_hash(name)) == -1) {
6668+
hash = PyObject_Hash(name);
6669+
if (hash == -1)
6670+
return -1;
6671+
}
6672+
return delitem_knownhash_lock_held((PyObject *)dict, name, hash);
6673+
} else {
6674+
return setitem_lock_held(dict, name, value);
6675+
}
6676+
}
6677+
66626678
// Called with either the object's lock or the dict's lock held
66636679
// depending on whether or not a dict has been materialized for
66646680
// the object.
@@ -6713,33 +6729,22 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values,
67136729
if (ix == DKIX_EMPTY) {
67146730
int res;
67156731
if (dict == NULL) {
6716-
dict = materialize_managed_dict_lock_held(obj);
6717-
if (dict == NULL) {
6732+
// Make the dict but don't publish it in the object
6733+
// so that no one else will see it.
6734+
dict = make_dict_from_instance_attributes(PyInterpreterState_Get(), keys, values);
6735+
if (dict == NULL ||
6736+
set_or_del_lock_held(dict, name, value) < 0) {
67186737
return -1;
67196738
}
67206739

6721-
if (value == NULL) {
6722-
res = PyDict_DelItem((PyObject *)dict, name);
6723-
} else {
6724-
res = PyDict_SetItem((PyObject *)dict, name, value);
6725-
}
6726-
6727-
return res;
6740+
FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict,
6741+
(PyDictObject *)dict);
6742+
return 0;
67286743
}
67296744

67306745
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(dict);
67316746

6732-
if (value == NULL) {
6733-
Py_hash_t hash;
6734-
if (!PyUnicode_CheckExact(name) || (hash = unicode_get_hash(name)) == -1) {
6735-
hash = PyObject_Hash(name);
6736-
if (hash == -1)
6737-
return -1;
6738-
}
6739-
res = delitem_knownhash_lock_held((PyObject *)dict, name, hash);
6740-
} else {
6741-
res = setitem_lock_held(dict, name, value);
6742-
}
6747+
res = set_or_del_lock_held (dict, name, value);
67436748
return res;
67446749
}
67456750

@@ -6782,11 +6787,7 @@ store_instance_attr_dict(PyObject *obj, PyDictObject *dict, PyObject *name, PyOb
67826787
res = store_instance_attr_lock_held(obj, values, name, value);
67836788
}
67846789
else {
6785-
if (value == NULL) {
6786-
res = PyDict_DelItem((PyObject *)dict, name);
6787-
} else {
6788-
res = PyDict_SetItem((PyObject *)dict, name, value);
6789-
}
6790+
res = set_or_del_lock_held(dict, name, value);
67906791
}
67916792
Py_END_CRITICAL_SECTION();
67926793
return res;
@@ -6870,9 +6871,8 @@ _PyObject_ManagedDictValidityCheck(PyObject *obj)
68706871
}
68716872
#endif
68726873

6873-
// Attempts to get an instance attribute from the inline values. Returns 0 if
6874-
// the lookup from the inline values was successful or 1 if the inline values
6875-
// are no longer valid. No error is set in either case.
6874+
// Attempts to get an instance attribute from the inline values. Returns true
6875+
// if successful, or false if the caller needs to lookup in the dictionary.
68766876
bool
68776877
_PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr)
68786878
{
@@ -7048,27 +7048,22 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
70487048

70497049
Py_BEGIN_CRITICAL_SECTION2(dict, obj);
70507050

7051-
// If the dict in the object has been replaced between when we
7052-
// got the dict and unlocked the objects then it's
7053-
// definitely no longer inline and there's no need to detach
7054-
// it, we can just replace it.
7051+
#ifdef Py_DEBUG
7052+
// If the dict in the object has been replaced between when we got
7053+
// the dict and unlocked the objects then it's definitely no longer
7054+
// inline and there's no need to detach it, we can just replace it.
7055+
// The call to _PyDict_DetachFromObject will be a nop.
70557056
PyDictObject *cur_dict = _PyObject_ManagedDictPointer(obj)->dict;
70567057
assert(cur_dict == dict ||
70577058
(cur_dict->ma_values != _PyObject_InlineValues(obj) &&
7059+
dict->ma_values != _PyObject_InlineValues(obj) &&
70587060
!_PyObject_InlineValues(obj)->valid));
7061+
#endif
70597062

7060-
Py_XINCREF(new_dict);
70617063
FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
70627064
(PyDictObject *)Py_XNewRef(new_dict));
70637065

7064-
// If we got a replacement dict after locking the object and the dict
7065-
// then the old dict had to already have been detached.
7066-
assert(cur_dict == dict ||
7067-
dict->ma_values != _PyObject_InlineValues(obj));
7068-
7069-
if (cur_dict == dict) {
7070-
_PyDict_DetachFromObject(dict, obj);
7071-
}
7066+
_PyDict_DetachFromObject(dict, obj);
70727067

70737068
Py_END_CRITICAL_SECTION2();
70747069

0 commit comments

Comments
 (0)