8000 Incref dict when it's usage is borrowed · python/cpython@16ca39f · GitHub
[go: up one dir, main page]
Skip to content

Commit 16ca39f

Browse files
DinoVDinoV
committed
Incref dict when it's usage is borrowed
1 parent ee1b8ce commit 16ca39f

File tree

8 files changed

+240
-46
lines changed

8 files changed

+240
-46
lines changed

‎Include/internal/pycore_gc.h

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -130,11 +130,7 @@ static inline int _PyObject_GC_IS_SHARED_INLINE(PyObject *op) {
130130
#define _PyObject_GC_IS_SHARED_INLINE(op) \
131131
_PyObject_GC_IS_SHARED_INLINE(_Py_CAST(PyObject*, op))
132132

133-
static inline void _PyObject_GC_SET_SHARED_INLINE(PyObject *op) {
134-
_PyObject_SET_GC_BITS(op, _PyGC_BITS_SHARED_INLINE);
135-
}
136-
#define _PyObject_GC_SET_SHARED_INLINE(op) \
137-
_PyObject_GC_SET_SHARED_INLINE(_Py_CAST(PyObject*, op))
133+
extern void _PyObject_GC_SET_SHARED_INLINE(PyObject *op);
138134

139135
#endif
140136

‎Include/refcount.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,10 @@ PyAPI_FUNC(void) _Py_DecRefSharedDebug(PyObject *, const char *, int);
274274
// zero. Otherwise, the thread gives up ownership and merges the reference
275275
// count fields.
276276
PyAPI_FUNC(void) _Py_MergeZeroLocalRefcount(PyObject *);
277+
278+
// Queue an object to be deferred at the next quiescent state
279+
PyAPI_FUNC(void) _PyObject_FreeDeferred(PyObject *o);
280+
277281
#endif
278282

279283
#if defined(Py_LIMITED_API) && (Py_LIMITED_API+0 >= 0x030c0000 || defined(Py_REF_DEBUG))

‎Lib/test/test_free_threading/test_dict.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,70 @@ def writer_func(l):
142142
for ref in thread_list:
143143
self.assertIsNone(ref())
144144

145+
def test_racing_set_object_dict(self):
146+
"""Races assigning to __dict__ should be thread safe"""
147+
class C: pass
148+
class MyDict(dict): pass
149+
for cyclic in (False, True):
150+
f = C()
151+
f.__dict__ = {"foo": 42}
152+
THREAD_COUNT = 10
153+
154+
def writer_func(l):
155+
for i in range(100000):
156+
if cyclic:
157+
other_d = {}
158+
d = MyDict({"foo": 100})
159+
if cyclic:
160+
d["x"] = other_d
161+
other_d["bar"] = d
162+
l.append(weakref.ref(d))
163+
f.__dict__ = d
164+
165+
def reader_func():
166+
for i in range(1000):
167+
f.foo
168+
169+
lists = []
170+
readers = []
171+
writers = []
172+
for x in range(THREAD_COUNT):
173+
thread_list = []
174+
lists.append(thread_list)
175+
writer = Thread(target=partial(writer_func, thread_list))
176+
writers.append(writer)
177+
178+
for x in range(THREAD_COUNT):
179+
reader = Thread(target=partial(reader_func))
180+
readers.append(reader)
181+
182+
for writer in writers:
183+
writer.start()
184+
for reader in readers:
185+
reader.start()
186+
187+
for writer in writers:
188+
writer.join()
189+
190+
for reader in readers:
191+
reader.join()
192+
193+
f.__dict__ = {}
194+
gc.collect()
195+
gc.collect()
196+
197+
count = 0
198+
ids = set()
199+
for thread_list in lists:
200+
for i, ref in enumerate(thread_list):
201+
if ref() is None:
202+
continue
203+
count += 1
204+
ids.add(id(ref()))
205+
count += 1
206+
207+
self.assertEqual(count, 0)
208+
145209
@unittest.skipIf(_testcapi is None, 'need _testcapi module')
146210
def test_dict_version(self):
147211
dict_version = _testcapi.dict_version

‎Objects/dictobject.c

Lines changed: 116 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,8 @@ ASSERT_DICT_LOCKED(PyObject *op)
165165

166166
#define IS_DICT_SHARED(mp) _PyObject_GC_IS_SHARED(mp)
167167
#define SET_DICT_SHARED(mp) _PyObject_GC_SET_SHARED(mp)
168+
#define IS_DICT_SHARED_INLINE(mp) _PyObject_GC_IS_SHARED_INLINE(mp)
169+
#define SET_DICT_SHARED_INLINE(mp) _PyObject_GC_SET_SHARED_INLINE(mp)
168170
#define LOAD_INDEX(keys, size, idx) _Py_atomic_load_int##size##_relaxed(&((const int##size##_t*)keys->dk_indices)[idx]);
169171
#define STORE_INDEX(keys, size, idx, value) _Py_atomic_store_int##size##_relaxed(&((int##size##_t*)keys->dk_indices)[idx], (int##size##_t)value);
170172
#define ASSERT_OWNED_OR_SHARED(mp) \
@@ -245,6 +247,8 @@ static inline void split_keys_entry_added(PyDictKeysObject *keys)
245247
#define UNLOCK_KEYS_IF_SPLIT(keys, kind)
246248
#define IS_DICT_SHARED(mp) (false)
247249
#define SET_DICT_SHARED(mp)
250+
#define IS_DICT_SHARED_INLINE(mp) (false)
251+
#define SET_DICT_SHARED_INLINE(mp)
248252
#define LOAD_INDEX(keys, size, idx) ((const int##size##_t*)(keys->dk_indices))[idx]
249253
#define STORE_INDEX(keys, size, idx, value) ((int##size##_t*)(keys->dk_indices))[idx] = (int##size##_t)value
250254

@@ -3117,7 +3121,7 @@ dict_dealloc(PyObject *self)
31173121
assert(keys->dk_refcnt == 1 || keys == Py_EMPTY_KEYS);
31183122
dictkeys_decref(interp, keys, false);
31193123
}
3120-
if (Py_IS_TYPE(mp, &PyDict_Type)) {
3124+
if (Py_IS_TYPE(mp, &PyDict_Type) && !IS_DICT_SHARED_INLINE(mp)) {
31213125
_Py_FREELIST_FREE(dicts, mp, Py_TYPE(mp)->tp_free);
31223126
}
31233127
else {
@@ -7030,6 +7034,75 @@ set_dict_inline_values(PyObject *obj, PyDictObject *new_dict)
70307034
}
70317035
}
70327036

7037+
#ifdef Py_GIL_DISABLED
7038+
7039+
// Trys and sets the dictionary for an object in the easy case when our current
7040+
// dictionary is either completely not materialized or is a dictionary which
7041+
// does not point at the inline values.
7042+
static bool
7043+
try_set_dict_inline_only_or_other_dict(PyObject *obj, PyObject *new_dict, PyDictObject **cur_dict)
7044+
{
7045+
bool replaced = false;
7046+
Py_BEGIN_CRITICAL_SECTION(obj);
7047+
7048+
PyDictObject *dict = *cur_dict = _PyObject_GetManagedDict(obj);
7049+
if (dict == NULL) {
7050+
// We only have inline values, we can just completely replace them.
7051+
set_dict_inline_values(obj, (PyDictObject *)new_dict);
7052+
replaced = true;
7053+
goto exit_lock;
7054+
}
7055+
7056+
if (FT_ATOMIC_LOAD_PTR_RELAXED(dict->ma_values) != _PyObject_InlineValues(obj)) {
7057+
// We have a materialized dict which doesn't point at the inline values,
7058+
// We get to simply swap dictionaries and free the old dictionary.
7059+
FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
7060+
(PyDictObject *)Py_XNewRef(new_dict));
7061+
replaced = true;
7062+
goto exit_lock;
7063+
} else {
7064+
// We have inline values, we need to lock the dict and the object
7065+
// at the same time to safely dematerialize them. To do that while releasing
7066+
// the object lock we need a strong reference to the current dictionary.
7067+
Py_INCREF(dict);
7068+
}
7069+
exit_lock:
7070+
Py_END_CRITICAL_SECTION();
7071+
return replaced;
7072+
}
7073+
7074+
#endif
7075+
7076+
// Replaces a dictionary that is probably the dictionary which has been
7077+
// materialized and points at the inline values. We could have raced
7078+
// and replaced it with another dictionary though.
7079+
static int
7080+
replace_dict_probably_inline_materialized(PyObject *obj, PyDictObject *inline_dict,
7081+
PyObject *new_dict, PyDictObject **replaced_dict)
7082+
{
7083+
// But we could have had another thread race in after we released
7084+
// the object lock
7085+
int err = 0;
7086+
*replaced_dict = _PyObject_GetManagedDict(obj);
7087+
assert(FT_ATOMIC_LOAD_PTR_RELAXED(inline_dict->ma_values) == _PyObject_InlineValues(obj));
7088+
7089+
if (*replaced_dict == inline_dict) {
7090+
err = _PyDict_DetachFromObject(inline_dict, obj);
7091+
if (err != 0) {
7092+
return err;
7093+
}
7094+
SET_DICT_SHARED_INLINE((PyObject *)inline_dict);
7095+
// We incref'd the inline dict and the object owns a ref.
7096+
// Clear the object's reference, we'll clear the local
7097+
// reference after releasing the lock.
7098+
Py_CLEAR(*replaced_dict);
7099+
}
7100+
7101+
FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
7102+
(PyDictObject *)Py_XNewRef(new_dict));
7103+
return err;
7104+
}
7105+
70337106
int
70347107
_PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
70357108
{
@@ -7038,42 +7111,51 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
70387111
int err = 0;
70397112
PyTypeObject *tp = Py_TYPE(obj);
70407113
if (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) {
7041-
PyDictObject *dict = _PyObject_GetManagedDict(obj);
7042-
if (dict == NULL) {
70437114
#ifdef Py_GIL_DISABLED
7044-
Py_BEGIN_CRITICAL_SECTION(obj);
7115+
PyDictObject *prev_dict;
7116+
if (!try_set_dict_inline_only_or_other_dict(obj, new_dict, &prev_dict)) {
7117+
// We had a materialized dictionary which pointed at the inline
7118+
// values. We need to lock both the object and the dict at the
7119+
// same time to safely replace it. We can't merely lock the dictionary
7120+
// while the object is locked because it could suspend the object lock.
7121+
PyDictObject *replaced_dict;
70457122

7046-
dict = _PyObject_ManagedDictPointer(obj)->dict;
7047-
if (dict == NULL) {
7048-
set_dict_inline_values(obj, (PyDictObject *)new_dict);
7049-
}
7123+
assert(prev_dict != NULL);
7124+
Py_BEGIN_CRITICAL_SECTION2(obj, prev_dict);
70507125

7051-
Py_END_CRITICAL_SECTION();
7126+
err = replace_dict_probably_inline_materialized(obj, prev_dict, new_dict, &replaced_dict);
70527127

7053-
if (dict == NULL) {
7054-
return 0;
7128+
Py_END_CRITICAL_SECTION2();
7129+
7130+
Py_DECREF(prev_dict);
7131+
if (err != 0) {
7132+
return err;
70557133
}
7134+
prev_dict = replaced_dict;
7135+
}
7136+
7137+
if (prev_dict != NULL) {
7138+
Py_BEGIN_CRITICAL_SECTION(prev_dict);
7139+
SET_DICT_SHARED_INLINE((PyObject *)prev_dict);
7140+
Py_END_CRITICAL_SECTION();
7141+
// Readers from the old dictionary use a borrowed reference. We need
7142+
// to set the dict to be freed via QSBR which requires locking it.
7143+
Py_DECREF(prev_dict);
7144+
}
7145+
return 0;
70567146
#else
7147+
PyDictObject *dict = _PyObject_GetManagedDict(obj);
7148+
if (dict == NULL) {
70577149
set_dict_inline_values(obj, (PyDictObject *)new_dict);
70587150
return 0;
7059-
#endif
7060-
}
7061-
7062-
Py_BEGIN_CRITICAL_SECTION2(dict, obj);
7063-
7064-
// We've locked dict, but the actual dict could have changed
7065-
// since we locked it.
7066-
dict = _PyObject_ManagedDictPointer(obj)->dict;
7067-
err = _PyDict_DetachFromObject(dict, obj);
7068-
if (err == 0) {
7069-
FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
7070-
(PyDictObject *)Py_XNewRef(new_dict));
70717151
}
7072-
Py_END_CRITICAL_SECTION2();
7073-
7074-
if (err == 0) {
7075-
Py_XDECREF(dict);
7152+
if (_PyDict_DetachFromObject(dict, obj) == 0) {
7153+
*_PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)Py_XNewRef(new_dict);
7154+
Py_DECREF(dict);
7155+
return 0;
70767156
}
7157+
return -1;
7158+
#endif
70777159
}
70787160
else {
70797161
PyDictObject *dict;
@@ -7086,7 +7168,13 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
70867168
(PyDictObject *)Py_XNewRef(new_dict));
70877169

70887170
Py_END_CRITICAL_SECTION();
7089-
7171+
#ifdef Py_GIL_DISABLED
7172+
if (dict != NULL) {
7173+
Py_BEGIN_CRITICAL_SECTION(dict);
7174+
SET_DICT_SHARED_INLINE((PyObject *)dict);
7175+
Py_END_CRITICAL_SECTION();
7176+
}
7177+
#endif
70907178
Py_XDECREF(dict);
70917179
}
70927180
assert(_PyObject_InlineValuesConsistencyCheck(obj));

‎Objects/object.c

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,27 @@ is_dead(PyObject *o)
319319
}
320320
# endif
321321

322+
void
323+
_PyObject_FreeDeferred(PyObject *o)
324+
{
325+
// It's possible that before we've hit the quiescent-state that it
326+
// has been incref'd and then decref'd back to zero again. We re-use
327+
// the finalized bit to track if we've already queued the delay freed
328+
// for this object as we no longer need to GC track it if it's being
329+
// freed by QSBR as it's reclaimed by the ref count.
330+
if (!_PyGC_FINALIZED(o)) {
331+
Py_BEGIN_CRITICAL_SECTION(o);
332+
if (!_PyGC_FINALIZED(o)) {
333+
if (_PyObject_GC_IS_TRACKED(o)) {
334+
PyObject_GC_UnTrack(o);
< C2EE code>335+
}
336+
_PyGC_SET_FINALIZED(o);
337+
_PyObject_FreeDelayed(o);
338+
}
339+
Py_END_CRITICAL_SECTION();
340+
}
341+
}
342+
322343
void
323344
_Py_DecRefSharedDebug(PyObject *o, const char *filename, int lineno)
324345
{
@@ -360,8 +381,12 @@ _Py_DecRefSharedDebug(PyObject *o, const char *filename, int lineno)
360381
_Py_brc_queue_object(o);
361382
}
362383
else if (new_shared == _Py_REF_MERGED) {
363-
// refcount is zero AND merged
364-
_Py_Dealloc(o);
384+
if (!_PyObject_GC_IS_SHARED_INLINE(o)) {
385+
// refcount is zero AND merged
386+
_Py_Dealloc(o);
387+
} else {
388+
_PyObject_FreeDeferred(o);
389+
}
365390
}
366391
}
367392

@@ -379,7 +404,12 @@ _Py_MergeZeroLocalRefcount(PyObject *op)
379404
Py_ssize_t shared = _Py_atomic_load_ssize_acquire(&op->ob_ref_shared);
380405
if (shared == 0) {
381406
// Fast-path: shared refcount is zero (including flags)
382-
_Py_Dealloc(op);
407+
if (!_PyObject_GC_IS_SHARED_INLINE(op)) {
408+
// refcount is zero AND merged
409+
_Py_Dealloc(op);
410+
} else {
411+
_PyObject_FreeDeferred(op);
412+
}
383413
return;
384414
}
385415

F438
@@ -398,7 +428,12 @@ _Py_MergeZeroLocalRefcount(PyObject *op)
398428
if (new_shared == _Py_REF_MERGED) {
399429
// i.e., the shared refcount is zero (only the flags are set) so we
400430
// deallocate the object.
401-
_Py_Dealloc(op);
431+
if (!_PyObject_GC_IS_SHARED_INLINE(op)) {
432+
// refcount is zero AND merged
433+
_Py_Dealloc(op);
434+
} else {
435+
_PyObject_FreeDeferred(op);
436+
}
402437
}
403438
}
404439

‎Objects/obmalloc.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1096,7 +1096,7 @@ static void
10961096
free_work_item(uintptr_t ptr)
10971097
{
10981098
if (ptr & 0x01) {
1099-
PyObject_Free((char *)(ptr - 1));
1099+
_Py_Dealloc((PyObject*)(char *)(ptr - 1));
11001100
}
11011101
else {
11021102
PyMem_Free((void *)ptr);

‎Python/brc.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,11 @@ merge_queued_objects(_PyObjectStack *to_merge)
107107
// Subtract one when merging because the queue had a reference.
108108
Py_ssize_t refcount = _Py_ExplicitMergeRefcount(ob, -1);
109109
if (refcount == 0) {
110-
_Py_Dealloc(ob);
110+
if (!_PyObject_GC_IS_SHARED_INLINE(ob)) {
111+
_Py_Dealloc(ob);
112+
} else {
113+
_PyObject_FreeDeferred(ob);
114+
}
111115
}
112116
}
113117
}

0 commit comments

Comments
 (0)
0