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

Commit 0fe4abc

Browse files
DinoVDinoV
committed
Incref dict when it's usage is borrowed
1 parent f3b2c36 commit 0fe4abc

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
@@ -277,6 +277,10 @@ PyAPI_FUNC(void) _Py_DecRefSharedDebug(PyObject *, const char *, int);
277277
// zero. Otherwise, the thread gives up ownership and merges the reference
278278
// count fields.
279279
PyAPI_FUNC(void) _Py_MergeZeroLocalRefcount(PyObject *);
280+
281+
// Queue an object to be deferred at the next quiescent state
282+
PyAPI_FUNC(void) _PyObject_FreeDeferred(PyObject *o);
283+
280284
#endif
281285

282286
#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

@@ -3198,7 +3202,7 @@ dict_dealloc(PyObject *self)
31983202
assert(keys->dk_refcnt == 1 || keys == Py_EMPTY_KEYS);
31993203
dictkeys_decref(interp, keys, false);
32003204
}
3201-
if (Py_IS_TYPE(mp, &PyDict_Type)) {
3205+
if (Py_IS_TYPE(mp, &PyDict_Type) && !IS_DICT_SHARED_INLINE(mp)) {
32023206
_Py_FREELIST_FREE(dicts, mp, Py_TYPE(mp)->tp_free);
32033207
}
32043208
else {
@@ -7126,6 +7130,75 @@ set_dict_inline_values(PyObject *obj, PyDictObject *new_dict)
71267130
}
71277131
}
71287132

7133+
#ifdef Py_GIL_DISABLED
7134+
7135+
// Trys and sets the dictionary for an object in the easy case when our current
7136+
// dictionary is either completely not materialized or is a dictionary which
7137+
// does not point at the inline values.
7138+
static bool
7139+
try_set_dict_inline_only_or_other_dict(PyObject *obj, PyObject *new_dict, PyDictObject **cur_dict)
7140+
{
7141+
bool replaced = false;
7142+
Py_BEGIN_CRITICAL_SECTION(obj);
7143+
7144+
PyDictObject *dict = *cur_dict = _PyObject_GetManagedDict(obj);
7145+
if (dict == NULL) {
7146+
// We only have inline values, we can just completely replace them.
7147+
set_dict_inline_values(obj, (PyDictObject *)new_dict);
7148+
replaced = true;
7149+
goto exit_lock;
7150+
}
7151+
7152+
if (FT_ATOMIC_LOAD_PTR_RELAXED(dict->ma_values) != _PyObject_InlineValues(obj)) {
7153+
// We have a materialized dict which doesn't point at the inline values,
7154+
// We get to simply swap dictionaries and free the old dictionary.
7155+
FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
7156+
(PyDictObject *)Py_XNewRef(new_dict));
7157+
replaced = true;
7158+
goto exit_lock;
7159+
} else {
7160+
// We have inline values, we need to lock the dict and the object
7161+
// at the same time to safely dematerialize them. To do that while releasing
7162+
// the object lock we need a strong reference to the current dictionary.
7163+
Py_INCREF(dict);
7164+
}
7165+
exit_lock:
7166+
Py_END_CRITICAL_SECTION();
7167+
return replaced;
7168+
}
7169+
7170+
#endif
7171+
7172+
// Replaces a dictionary that is probably the dictionary which has been
7173+
// materialized and points at the inline values. We could have raced
7174+
// and replaced it with another dictionary though.
7175+
static int
7176+
replace_dict_probably_inline_materialized(PyObject *obj, PyDictObject *inline_dict,
7177+
PyObject *new_dict, PyDictObject **replaced_dict)
7178+
{
7179+
// But we could have had another thread race in after we released
7180+
// the object lock
7181+
int err = 0;
7182+
*replaced_dict = _PyObject_GetManagedDict(obj);
7183+
assert(FT_ATOMIC_LOAD_PTR_RELAXED(inline_dict->ma_values) == _PyObject_InlineValues(obj));
7184+
7185+
if (*replaced_dict == inline_dict) {
7186+
err = _PyDict_DetachFromObject(inline_dict, obj);
7187+
if (err != 0) {
7188+
return err;
7189+
}
7190+
SET_DICT_SHARED_INLINE((PyObject *)inline_dict);
7191+
// We incref'd the inline dict and the object owns a ref.
7192+
// Clear the object's reference, we'll clear the local
7193+
// reference after releasing the lock.
7194+
Py_CLEAR(*replaced_dict);
7195+
}
7196+
7197+
FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
7198+
(PyDictObject *)Py_XNewRef(new_dict));
7199+
return err;
7200+
}
7201+
71297202
int
71307203
_PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
71317204
{
@@ -7134,42 +7207,51 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
71347207
int err = 0;
71357208
PyTypeObject *tp = Py_TYPE(obj);
71367209
if (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) {
7137-
PyDictObject *dict = _PyObject_GetManagedDict(obj);
7138-
if (dict == NULL) {
71397210
#ifdef Py_GIL_DISABLED
7140-
Py_BEGIN_CRITICAL_SECTION(obj);
7211+
PyDictObject *prev_dict;
7212+
if (!try_set_dict_inline_only_or_other_dict(obj, new_dict, &prev_dict)) {
7213+
// We had a materialized dictionary which pointed at the inline
7214+
// values. We need to lock both the object and the dict at the
7215+
// same time to safely replace it. We can't merely lock the dictionary
7216+
// while the object is locked because it could suspend the object lock.
7217+
PyDictObject *replaced_dict;
71417218

7142-
dict = _PyObject_ManagedDictPointer(obj)->dict;
7143-
if (dict == NULL) {
7144-
set_dict_inline_values(obj, (PyDictObject *)new_dict);
7145-
}
7219+
assert(prev_dict != NULL);
7220+
Py_BEGIN_CRITICAL_SECTION2(obj, prev_dict);
71467221

7147-
Py_END_CRITICAL_SECTION();
7222+
err = replace_dict_probably_inline_materialized(obj, prev_dict, new_dict, &replaced_dict);
71487223

7149-
if (dict == NULL) {
7150-
return 0;
7224+
Py_END_CRITICAL_SECTION2();
7225+
7226+
Py_DECREF(prev_dict);
7227+
if (err != 0) {
7228+
return err;
71517229
}
7230+
prev_dict = replaced_dict;
7231+
}
7232+
7233+
if (prev_dict != NULL) {
7234+
Py_BEGIN_CRITICAL_SECTION(prev_dict);
7235+
SET_DICT_SHARED_INLINE((PyObject *)prev_dict);
7236+
Py_END_CRITICAL_SECTION();
7237+
// Readers from the old dictionary use a borrowed reference. We need
7238+
// to set the dict to be freed via QSBR which requires locking it.
7239+
Py_DECREF(prev_dict);
7240+
}
7241+
return 0;
71527242
#else
7243+
PyDictObject *dict = _PyObject_GetManagedDict(obj);
7244+
if (dict == NULL) {
71537245
set_dict_inline_values(obj, (PyDictObject *)new_dict);
71547246
return 0;
7155-
#endif
7156-
}
7157-
7158-
Py_BEGIN_CRITICAL_SECTION2(dict, obj);
7159-
7160-
// We've locked dict, but the actual dict could have changed
7161-
// since we locked it.
7162-
dict = _PyObject_ManagedDictPointer(obj)->dict;
7163-
err = _PyDict_DetachFromObject(dict, obj);
7164-
if (err == 0) {
7165-
FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
7166-
(PyDictObject *)Py_XNewRef(new_dict));
71677247
}
7168-
Py_END_CRITICAL_SECTION2();
7169-
7170-
if (err == 0) {
7171-
Py_XDECREF(dict);
7248+
if (_PyDict_DetachFromObject(dict, obj) == 0) {
7249+
_PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)Py_XNewRef(new_dict);
7250+
Py_DECREF(dict);
7251+
return 0;
71727252
}
7253+
return -1;
7254+
#endif
71737255
}
71747256
else {
71757257
PyDictObject *dict;
@@ -7182,7 +7264,13 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
71827264
(PyDictObject *)Py_XNewRef(new_dict));
71837265

71847266
Py_END_CRITICAL_SECTION();
7185-
7267+
#ifdef Py_GIL_DISABLED
7268+
if (dict != NULL) {
7269+
Py_BEGIN_CRITICAL_SECTION(dict);
7270+
SET_DICT_SHARED_INLINE((PyObject *)dict);
7271+
Py_END_CRITICAL_SECTION();
7272+
}
7273+
#endif
71867274
Py_XDECREF(dict);
71877275
}
71887276
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);
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

@@ -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