From afddb7080f024994ded39ad751056066fbcaa6e2 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Wed, 31 Jan 2024 17:40:43 -0800 Subject: [PATCH 1/7] Thread safe lock-free iteration --- Include/internal/pycore_dict.h | 1 + Objects/dictobject.c | 309 +++++++++++++++++++++++++++++---- 2 files changed, 275 insertions(+), 35 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index 5a496d59ab7af7..d1a0010b9a81dd 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -212,6 +212,7 @@ static inline PyDictUnicodeEntry* DK_UNICODE_ENTRIES(PyDictKeysObject *dk) { #define DICT_WATCHER_AND_MODIFICATION_MASK ((1 << (DICT_MAX_WATCHERS + DICT_WATCHED_MUTATION_BITS)) - 1) #define DICT_VALUES_SIZE(values) ((uint8_t *)values)[-1] +#define DICT_VALUES_USED_SIZE(values) ((uint8_t *)values)[-2] #ifdef Py_GIL_DISABLED #define DICT_NEXT_VERSION(INTERP) \ diff --git a/Objects/dictobject.c b/Objects/dictobject.c index ed92998a23a768..44fde4ac7729d5 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -158,6 +158,15 @@ ASSERT_DICT_LOCKED(PyObject *op) #define STORE_INDEX(keys, size, idx, value) _Py_atomic_store_int##size##_relaxed(&((int##size##_t*)keys->dk_indices)[idx], (int##size##_t)value); #define ASSERT_OWNED_OR_SHARED(mp) \ assert(_Py_IsOwnedByCurrentThread((PyObject *)mp) || IS_DICT_SHARED(mp)); +#define LOAD_KEYS_NENTRIES(d) +#define DECREMENT(v) _Py_atomic_add_ssize(&(v), -1) + +static inline Py_ssize_t +load_keys_nentries(PyDictObject *mp) +{ + PyDictKeysObject *keys = _Py_atomic_load_ptr(&mp->ma_keys); + return _Py_atomic_load_ssize(&keys->dk_nentries); +} static inline void set_keys(PyDictObject *mp, PyDictKeysObject *keys) @@ -213,6 +222,7 @@ static inline void split_keys_entry_added(PyDictKeysObject *keys) #define SET_DICT_SHARED(mp) #define LOAD_INDEX(keys, size, idx) ((const int##size##_t*)(keys->dk_indices))[idx] #define STORE_INDEX(keys, size, idx, value) ((int##size##_t*)(keys->dk_indices))[idx] = (int##size##_t)value +#define DECREMENT(v) ((v)--) static inline void split_keys_entry_added(PyDictKeysObject *keys) { @@ -232,6 +242,13 @@ set_values(PyDictObject *mp, PyDictValues *values) mp->ma_values = values; } +static inline Py_ssize_t +load_keys_nentries(PyDictObject *mp) +{ + return mp->ma_keys->dk_nentries; +} + + #endif #define PERTURB_SHIFT 5 @@ -4858,7 +4875,7 @@ dictiter_new(PyDictObject *dict, PyTypeObject *itertype) di->di_pos = dict->ma_used - 1; } else { - di->di_pos = dict->ma_keys->dk_nentries - 1; + di->di_pos = load_keys_nentries(dict) - 1; } } else { @@ -4925,6 +4942,16 @@ static PyMethodDef dictiter_methods[] = { {NULL, NULL} /* sentinel */ }; +#ifdef Py_GIL_DISABLED + +static int +dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, + PyObject **out_key, PyObject **out_value); + +#endif + +#ifndef Py_GIL_DISABLED + static PyObject* dictiter_iternextkey_lock_held(PyDictObject *d, PyObject *self) { @@ -4992,6 +5019,8 @@ dictiter_iternextkey_lock_held(PyDictObject *d, PyObject *self) return NULL; } +#endif + static PyObject* dictiter_iternextkey(PyObject *self) { @@ -5002,9 +5031,13 @@ dictiter_iternextkey(PyObject *self) return NULL; PyObject *value; - Py_BEGIN_CRITICAL_SECTION(d); +#ifdef Py_GIL_DISABLED + if (!dictiter_iternext_threadsafe(d, self, &value, NULL)) { + value = NULL; + } +#else value = dictiter_iternextkey_lock_held(d, self); - Py_END_CRITICAL_SECTION(); +#endif return value; } @@ -5042,6 +5075,8 @@ PyTypeObject PyDictIterKey_Type = { 0, }; +#ifndef Py_GIL_DISABLED + static PyObject * dictiter_iternextvalue_lock_held(PyDictObject *d, PyObject *self) { @@ -5107,6 +5142,8 @@ dictiter_iternextvalue_lock_held(PyDictObject *d, PyObject *self) return NULL; } +#endif + static PyObject * dictiter_iternextvalue(PyObject *self) { @@ -5117,9 +5154,13 @@ dictiter_iternextvalue(PyObject *self) return NULL; PyObject *value; - Py_BEGIN_CRITICAL_SECTION(d); +#ifdef Py_GIL_DISABLED + if (!dictiter_iternext_threadsafe(d, self, NULL, &value)) { + value = NULL; + } +#else value = dictiter_iternextvalue_lock_held(d, self); - Py_END_CRITICAL_SECTION(); +#endif return value; } @@ -5157,11 +5198,12 @@ PyTypeObject PyDictIterValue_Type = { 0, }; -static PyObject * -dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self) +static int +dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self, + PyObject **out_key, PyObject **out_value) { dictiterobject *di = (dictiterobject *)self; - PyObject *key, *value, *result; + PyObject *key, *value; Py_ssize_t i; assert (PyDict_Check(d)); @@ -5170,10 +5212,19 @@ dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self) PyErr_SetString(PyExc_RuntimeError, "dictionary changed size during iteration"); di->di_used = -1; /* Make this state sticky */ - return NULL; + return 0; } +#ifdef Py_GIL_DISABLED + Py_ssize_t start_pos; + // Even though we hold the lock here we may still lose a race against + // a lock-free iterator, therefore we may end up retrying our iteration. +retry: + start_pos = i = _Py_atomic_load_ssize_relaxed(&di->di_pos); +#else i = di->di_pos; +#endif + assert(i >= 0); if (_PyDict_HasSplitTable(d)) { if (i >= d->ma_used) @@ -5214,36 +5265,198 @@ dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self) "dictionary keys changed during iteration"); goto fail; } +#ifdef Py_GIL_DISABLED + if (!_Py_atomic_compare_exchange_ssize(&di->di_pos, &start_pos, i+1)) { + // We lost a a race with a lock-free iterator! + goto retry; + } +#else di->di_pos = i+1; - di->len--; - result = di->di_result; - if (Py_REFCNT(result) == 1) { - PyObject *oldkey = PyTuple_GET_ITEM(result, 0); - PyObject *oldvalue = PyTuple_GET_ITEM(result, 1); - PyTuple_SET_ITEM(result, 0, Py_NewRef(key)); - PyTuple_SET_ITEM(result, 1, Py_NewRef(value)); - Py_INCREF(result); - Py_DECREF(oldkey); - Py_DECREF(oldvalue); - // bpo-42536: The GC may have untracked this result tuple. Since we're - // recycling it, make sure it's tracked again: - if (!_PyObject_GC_IS_TRACKED(result)) { - _PyObject_GC_TRACK(result); +#endif + DECREMENT(di->len); + if (out_key != NULL) { + *out_key = Py_NewRef(key); + } + if (out_value != NULL) { + *out_value = Py_NewRef(value); + } + return 1; + +fail: + di->di_dict = NULL; + Py_DECREF(d); + return 0; +} + +#ifdef Py_GIL_DISABLED + +// Grabs the key and/or value from the provided locations and if successful +// returns them with an increased reference count. If either one is unsucessful +// nothing is incref'd and returns 0. +static int +acquire_key_value(PyObject **key_loc, PyObject *value, PyObject **value_loc, + PyObject **out_key, PyObject **out_value) +{ + if (out_key) { + *out_key = _Py_TryXGetRef(key_loc); + if (*out_key == NULL) { + return 0; + } + } + + if (out_value) { + if (!_Py_TryIncref(value_loc, value)) { + if (out_key) { + Py_DECREF(*out_key); + } + return 0; + } + *out_value = value; + } + + return 1; +} + +static Py_ssize_t +load_values_used_size(PyDictValues *values) +{ + return (Py_ssize_t)_Py_atomic_load_uint8(&DICT_VALUES_USED_SIZE(values)); +} + +static int +dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, + PyObject **out_key, PyObject **out_value) +{ + dictiterobject *di = (dictiterobject *)self; + Py_ssize_t i; + PyDictKeysObject *k; + + assert (PyDict_Check(d)); + + if (di->di_used != _Py_atomic_load_ssize_relaxed(&d->ma_used)) { + PyErr_SetString(PyExc_RuntimeError, + "dictionary changed size during iteration"); + di->di_used = -1; /* Make this state sticky */ + return 0; + } + + ensure_shared_on_read(d); + + Py_ssize_t start_pos = i = _Py_atomic_load_ssize_relaxed(&di->di_pos); + k = _Py_atomic_load_ptr_relaxed(&d->ma_keys); + assert(i >= 0); + if (_PyDict_HasSplitTable(d)) { + PyDictValues *values = _Py_atomic_load_ptr_relaxed(&d->ma_values); + if (values == NULL) + goto concurrent_modification; + + Py_ssize_t used = load_values_used_size(values); + if (i >= used) + goto fail; + + // We're racing against writes to the order from delete_index_from_values, but + // single threaded can suffer from concurrent modification to those as well and + // can have either duplicated or skipped attributes, so we strive to do no better + // here. + int index = get_index_from_order(d, i); + PyObject *value = _Py_atomic_load_ptr(&values->values[index]); + if (!acquire_key_value(&DK_UNICODE_ENTRIES(k)[index].me_key, value, + &values->values[index], out_key, out_value)) { + goto try_locked; } } else { - result = PyTuple_New(2); - if (result == NULL) - return NULL; - PyTuple_SET_ITEM(result, 0, Py_NewRef(key)); - PyTuple_SET_ITEM(result, 1, Py_NewRef(value)); + Py_ssize_t n = _Py_atomic_load_ssize_relaxed(&k->dk_nentries); + if (DK_IS_UNICODE(k)) { + PyDictUnicodeEntry *entry_ptr = &DK_UNICODE_ENTRIES(k)[i]; + PyObject *value; + while (i < n && + (value = _Py_atomic_load_ptr(&entry_ptr->me_value)) == NULL) { + entry_ptr++; + i++; + } + if (i >= n) + goto fail; + + if (!acquire_key_value(&entry_ptr->me_key, value, + &entry_ptr->me_value, out_key, out_value)) { + goto try_locked; + } + } + else { + PyDictKeyEntry *entry_ptr = &DK_ENTRIES(k)[i]; + PyObject *value; + while (i < n && + (value = _Py_atomic_load_ptr(&entry_ptr->me_value)) == NULL) { + entry_ptr++; + i++; + } + + if (i >= n) + goto fail; + + if (!acquire_key_value(&entry_ptr->me_key, value, + &entry_ptr->me_value, out_key, out_value)) { + goto try_locked; + } + } } - return result; + // We found an element (key), but did not expect it + if (_Py_atomic_load_ssize_relaxed(&di->len) == 0) { + goto concurrent_modification; + } + if (!_Py_atomic_compare_exchange_ssize(&di->di_pos, &start_pos, i+1)) { + // We lost a race with someone else iterating... + if (out_key != NULL) { + Py_DECREF(*out_key); + } + if (out_value != NULL) { + Py_DECREF(*out_value); + } + goto try_locked; + } + _Py_atomic_add_ssize(&di->len, -1); + return 1; + +concurrent_modification: + PyErr_SetString(PyExc_RuntimeError, + "dictionary keys changed during iteration"); fail: di->di_dict = NULL; Py_DECREF(d); - return NULL; + return 0; + + int success; +try_locked: + Py_BEGIN_CRITICAL_SECTION(d); + success = dictiter_iternextitem_lock_held(d, self, out_key, out_value); + Py_END_CRITICAL_SECTION(); + return success; +} + +#endif + +static bool +acquire_iter_result(PyObject *result) +{ +#ifdef Py_GIL_DISABLED + if (Py_REFCNT(result) == 1) { + Py_INCREF(result); + if (Py_REFCNT(result) == 2) { + return true; + } + Py_DECREF(result); + return false; + } + return false; +#else + if (Py_REFCNT(result) == 1) { + Py_INCREF(result); + return true; + } + return false; +#endif } static PyObject * @@ -5255,11 +5468,37 @@ dictiter_iternextitem(PyObject *self) if (d == NULL) return NULL; - PyObject *item; - Py_BEGIN_CRITICAL_SECTION(d); - item = dictiter_iternextitem_lock_held(d, self); - Py_END_CRITICAL_SECTION(); - return item; + PyObject *key, *value; +#ifdef Py_GIL_DISABLED + if (dictiter_iternext_threadsafe(d, self, &key, &value)) { +#else + if (dictiter_iternextitem_lock_held(d, self, &key, &value)) { + +#endif + PyObject *result = di->di_result; + if (acquire_iter_result(result)) { + PyObject *oldkey = PyTuple_GET_ITEM(result, 0); + PyObject *oldvalue = PyTuple_GET_ITEM(result, 1); + PyTuple_SET_ITEM(result, 0, key); + PyTuple_SET_ITEM(result, 1, value); + Py_DECREF(oldkey); + Py_DECREF(oldvalue); + // bpo-42536: The GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: + if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } + } + else { + result = PyTuple_New(2); + if (result == NULL) + return NULL; + PyTuple_SET_ITEM(result, 0, key); + PyTuple_SET_ITEM(result, 1, value); + } + return result; + } + return NULL; } PyTypeObject PyDictIterItem_Type = { From 39bc495d4dd3ba2e085decd20c6f72e1b65689a1 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Thu, 15 Feb 2024 16:40:46 -0800 Subject: [PATCH 2/7] Don't use atomics for iteratio and allow multiple threads to race on values seen --- Objects/dictobject.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 44fde4ac7729d5..941a0655318e66 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -5342,7 +5342,7 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, ensure_shared_on_read(d); - Py_ssize_t start_pos = i = _Py_atomic_load_ssize_relaxed(&di->di_pos); + i = _Py_atomic_load_ssize_relaxed(&di->di_pos); k = _Py_atomic_load_ptr_relaxed(&d->ma_keys); assert(i >= 0); if (_PyDict_HasSplitTable(d)) { @@ -5402,20 +5402,13 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, } } // We found an element (key), but did not expect it - if (_Py_atomic_load_ssize_relaxed(&di->len) == 0) { + Py_ssize_t len; + if ((len = _Py_atomic_load_ssize_relaxed(&di->len)) == 0) { goto concurrent_modification; } - if (!_Py_atomic_compare_exchange_ssize(&di->di_pos, &start_pos, i+1)) { - // We lost a race with someone else iterating... - if (out_key != NULL) { - Py_DECREF(*out_key); - } - if (out_value != NULL) { - Py_DECREF(*out_value); - } - goto try_locked; - } - _Py_atomic_add_ssize(&di->len, -1); + + _Py_atomic_store_ssize_relaxed(&di->di_pos, i + 1); + _Py_atomic_store_ssize_relaxed(&di->len, len - 1); return 1; concurrent_modification: @@ -5441,22 +5434,16 @@ static bool acquire_iter_result(PyObject *result) { #ifdef Py_GIL_DISABLED - if (Py_REFCNT(result) == 1) { - Py_INCREF(result); - if (Py_REFCNT(result) == 2) { - return true; - } - Py_DECREF(result); - return false; - } - return false; + if (_Py_IsOwnedByCurrentThread(result) && + result->ob_ref_local == 1 && + _Py_atomic_load_ssize_relaxed(&result->ob_ref_shared) == 0) { #else if (Py_REFCNT(result) == 1) { +#endif Py_INCREF(result); return true; } return false; -#endif } static PyObject * From 902ce67df892d64d6e9eec567a41c8ba6a2f95c1 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Fri, 16 Feb 2024 09:49:01 -0800 Subject: [PATCH 3/7] Fixup ifdef/ifndef --- Objects/dictobject.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 941a0655318e66..d2524634ade009 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -4948,9 +4948,7 @@ static int dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, PyObject **out_key, PyObject **out_value); -#endif - -#ifndef Py_GIL_DISABLED +#else /* Py_GIL_DISABLED */ static PyObject* dictiter_iternextkey_lock_held(PyDictObject *d, PyObject *self) From 0ba2f5d439f693ee2dd8085de9e6eaef649e10e7 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Fri, 16 Feb 2024 11:06:27 -0800 Subject: [PATCH 4/7] Use has_unique_reference --- Objects/dictobject.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index d2524634ade009..d3182d22f5a4ce 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -5428,13 +5428,23 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, #endif +static bool +has_unique_reference(PyObject *op) +{ +#ifdef Py_GIL_DISABLED + return (_Py_IsOwnedByCurrentThread(op) && + op->ob_ref_local == 1 && + _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared) == 0); +#else + return Py_REFCNT(op) == 1; +#endif +} + static bool acquire_iter_result(PyObject *result) { #ifdef Py_GIL_DISABLED - if (_Py_IsOwnedByCurrentThread(result) && - result->ob_ref_local == 1 && - _Py_atomic_load_ssize_relaxed(&result->ob_ref_shared) == 0) { + if (has_unique_reference(result)) { #else if (Py_REFCNT(result) == 1) { #endif @@ -6647,18 +6657,6 @@ _PyObject_MakeDictFromInstanceAttributes(PyObject *obj, PyDictValues *values) return make_dict_from_instance_attributes(interp, keys, values); } -static bool -has_unique_reference(PyObject *op) -{ -#ifdef Py_GIL_DISABLED - return (_Py_IsOwnedByCurrentThread(op) && - op->ob_ref_local == 1 && - _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared) == 0); -#else - return Py_REFCNT(op) == 1; -#endif -} - // Return true if the dict was dematerialized, false otherwise. bool _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv) From 042cf26bb76753f145f1f8ef42c5705bf1899f91 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Wed, 21 Feb 2024 13:05:15 -0800 Subject: [PATCH 5/7] Less atomics --- Objects/dictobject.c | 52 +++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index d3182d22f5a4ce..99baae83f2f68b 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -115,19 +115,20 @@ As a consequence of this, split keys have a maximum size of 16. #define PyDict_MINSIZE 8 #include "Python.h" -#include "pycore_bitutils.h" // _Py_bit_length -#include "pycore_call.h" // _PyObject_CallNoArgs() -#include "pycore_ceval.h" // _PyEval_GetBuiltin() -#include "pycore_code.h" // stats -#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION, Py_END_CRITICAL_SECTION -#include "pycore_dict.h" // export _PyDict_SizeOf() -#include "pycore_freelist.h" // _PyFreeListState_GET() -#include "pycore_gc.h" // _PyObject_GC_IS_TRACKED() -#include "pycore_object.h" // _PyObject_GC_TRACK(), _PyDebugAllocatorStats() -#include "pycore_pyerrors.h" // _PyErr_GetRaisedException() -#include "pycore_pystate.h" // _PyThreadState_GET() -#include "pycore_setobject.h" // _PySet_NextEntry() -#include "stringlib/eq.h" // unicode_eq() +#include "pycore_bitutils.h" // _Py_bit_length +#include "pycore_call.h" // _PyObject_CallNoArgs() +#include "pycore_ceval.h" // _PyEval_GetBuiltin() +#include "pycore_code.h" // stats +#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION, Py_END_CRITICAL_SECTION +#include "pycore_dict.h" // export _PyDict_SizeOf() +#include "pycore_freelist.h" // _PyFreeListState_GET() +#include "pycore_gc.h" // _PyObject_GC_IS_TRACKED() +#include "pycore_object.h" // _PyObject_GC_TRACK(), _PyDebugAllocatorStats() +#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_LOAD_SSIZE_RELAXED +#include "pycore_pyerrors.h" // _PyErr_GetRaisedException() +#include "pycore_pystate.h" // _PyThreadState_GET() +#include "pycore_setobject.h" // _PySet_NextEntry() +#include "stringlib/eq.h" // unicode_eq() #include @@ -159,7 +160,6 @@ ASSERT_DICT_LOCKED(PyObject *op) #define ASSERT_OWNED_OR_SHARED(mp) \ assert(_Py_IsOwnedByCurrentThread((PyObject *)mp) || IS_DICT_SHARED(mp)); #define LOAD_KEYS_NENTRIES(d) -#define DECREMENT(v) _Py_atomic_add_ssize(&(v), -1) static inline Py_ssize_t load_keys_nentries(PyDictObject *mp) @@ -222,7 +222,6 @@ static inline void split_keys_entry_added(PyDictKeysObject *keys) #define SET_DICT_SHARED(mp) #define LOAD_INDEX(keys, size, idx) ((const int##size##_t*)(keys->dk_indices))[idx] #define STORE_INDEX(keys, size, idx, value) ((int##size##_t*)(keys->dk_indices))[idx] = (int##size##_t)value -#define DECREMENT(v) ((v)--) static inline void split_keys_entry_added(PyDictKeysObject *keys) { @@ -5017,7 +5016,7 @@ dictiter_iternextkey_lock_held(PyDictObject *d, PyObject *self) return NULL; } -#endif +#endif /* Py_GIL_DISABLED */ static PyObject* dictiter_iternextkey(PyObject *self) @@ -5140,7 +5139,7 @@ dictiter_iternextvalue_lock_held(PyDictObject *d, PyObject *self) return NULL; } -#endif +#endif /* Py_GIL_DISABLED */ static PyObject * dictiter_iternextvalue(PyObject *self) @@ -5213,15 +5212,7 @@ dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self, return 0; } -#ifdef Py_GIL_DISABLED - Py_ssize_t start_pos; - // Even though we hold the lock here we may still lose a race against - // a lock-free iterator, therefore we may end up retrying our iteration. -retry: - start_pos = i = _Py_atomic_load_ssize_relaxed(&di->di_pos); -#else - i = di->di_pos; -#endif + i = FT_ATOMIC_LOAD_SSIZE_RELAXED(di->di_pos); assert(i >= 0); if (_PyDict_HasSplitTable(d)) { @@ -5263,15 +5254,8 @@ dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self, "dictionary keys changed during iteration"); goto fail; } -#ifdef Py_GIL_DISABLED - if (!_Py_atomic_compare_exchange_ssize(&di->di_pos, &start_pos, i+1)) { - // We lost a a race with a lock-free iterator! - goto retry; - } -#else di->di_pos = i+1; -#endif - DECREMENT(di->len); + di->len--; if (out_key != NULL) { *out_key = Py_NewRef(key); } From a079a6d19f1d54c634c25704d8a1e9804417a798 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Wed, 21 Feb 2024 13:59:01 -0800 Subject: [PATCH 6/7] Fix style issues --- Objects/dictobject.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 99baae83f2f68b..0f7b8de8e752fd 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -5029,7 +5029,7 @@ dictiter_iternextkey(PyObject *self) PyObject *value; #ifdef Py_GIL_DISABLED - if (!dictiter_iternext_threadsafe(d, self, &value, NULL)) { + if (!dictiter_iternext_threadsafe(d, self, &value, NULL) == 0) { value = NULL; } #else @@ -5152,7 +5152,7 @@ dictiter_iternextvalue(PyObject *self) PyObject *value; #ifdef Py_GIL_DISABLED - if (!dictiter_iternext_threadsafe(d, self, NULL, &value)) { + if (!dictiter_iternext_threadsafe(d, self, NULL, &value) == 0) { value = NULL; } #else @@ -5209,7 +5209,7 @@ dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self, PyErr_SetString(PyExc_RuntimeError, "dictionary changed size during iteration"); di->di_used = -1; /* Make this state sticky */ - return 0; + return -1; } i = FT_ATOMIC_LOAD_SSIZE_RELAXED(di->di_pos); @@ -5262,12 +5262,12 @@ dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self, if (out_value != NULL) { *out_value = Py_NewRef(value); } - return 1; + return 0; fail: di->di_dict = NULL; Py_DECREF(d); - return 0; + return -1; } #ifdef Py_GIL_DISABLED @@ -5319,7 +5319,7 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, PyErr_SetString(PyExc_RuntimeError, "dictionary changed size during iteration"); di->di_used = -1; /* Make this state sticky */ - return 0; + return -1; } ensure_shared_on_read(d); @@ -5329,12 +5329,14 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, assert(i >= 0); if (_PyDict_HasSplitTable(d)) { PyDictValues *values = _Py_atomic_load_ptr_relaxed(&d->ma_values); - if (values == NULL) + if (values == NULL) { goto concurrent_modification; + } Py_ssize_t used = load_values_used_size(values); - if (i >= used) + if (i >= used) { goto fail; + } // We're racing against writes to the order from delete_index_from_values, but // single threaded can suffer from concurrent modification to those as well and @@ -5391,7 +5393,7 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, _Py_atomic_store_ssize_relaxed(&di->di_pos, i + 1); _Py_atomic_store_ssize_relaxed(&di->len, len - 1); - return 1; + return 0; concurrent_modification: PyErr_SetString(PyExc_RuntimeError, @@ -5400,14 +5402,14 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, fail: di->di_dict = NULL; Py_DECREF(d); - return 0; + return -1; - int success; + int res; try_locked: Py_BEGIN_CRITICAL_SECTION(d); - success = dictiter_iternextitem_lock_held(d, self, out_key, out_value); + res = dictiter_iternextitem_lock_held(d, self, out_key, out_value); Py_END_CRITICAL_SECTION(); - return success; + return res; } #endif @@ -5427,11 +5429,7 @@ has_unique_reference(PyObject *op) static bool acquire_iter_result(PyObject *result) { -#ifdef Py_GIL_DISABLED if (has_unique_reference(result)) { -#else - if (Py_REFCNT(result) == 1) { -#endif Py_INCREF(result); return true; } @@ -5449,9 +5447,9 @@ dictiter_iternextitem(PyObject *self) PyObject *key, *value; #ifdef Py_GIL_DISABLED - if (dictiter_iternext_threadsafe(d, self, &key, &value)) { + if (dictiter_iternext_threadsafe(d, self, &key, &value) == 0) { #else - if (dictiter_iternextitem_lock_held(d, self, &key, &value)) { + if (dictiter_iternextitem_lock_held(d, self, &key, &value) == 0) { #endif PyObject *result = di->di_result; From a1d7718c41d77640c2f9f69ce303195165ab41ff Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Thu, 22 Feb 2024 10:32:00 -0800 Subject: [PATCH 7/7] acquire_key_value returns -1 and 0 --- Objects/dictobject.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 0f7b8de8e752fd..5ae4c3dbea2380 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -5274,7 +5274,7 @@ dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self, // Grabs the key and/or value from the provided locations and if successful // returns them with an increased reference count. If either one is unsucessful -// nothing is incref'd and returns 0. +// nothing is incref'd and returns -1. static int acquire_key_value(PyObject **key_loc, PyObject *value, PyObject **value_loc, PyObject **out_key, PyObject **out_value) @@ -5282,7 +5282,7 @@ acquire_key_value(PyObject **key_loc, PyObject *value, PyObject **value_loc, if (out_key) { *out_key = _Py_TryXGetRef(key_loc); if (*out_key == NULL) { - return 0; + return -1; } } @@ -5291,12 +5291,12 @@ acquire_key_value(PyObject **key_loc, PyObject *value, PyObject **value_loc, if (out_key) { Py_DECREF(*out_key); } - return 0; + return -1; } *out_value = value; } - return 1; + return 0; } static Py_ssize_t @@ -5344,8 +5344,8 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, // here. int index = get_index_from_order(d, i); PyObject *value = _Py_atomic_load_ptr(&values->values[index]); - if (!acquire_key_value(&DK_UNICODE_ENTRIES(k)[index].me_key, value, - &values->values[index], out_key, out_value)) { + if (acquire_key_value(&DK_UNICODE_ENTRIES(k)[index].me_key, value, + &values->values[index], out_key, out_value) < 0) { goto try_locked; } } @@ -5362,8 +5362,8 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, if (i >= n) goto fail; - if (!acquire_key_value(&entry_ptr->me_key, value, - &entry_ptr->me_value, out_key, out_value)) { + if (acquire_key_value(&entry_ptr->me_key, value, + &entry_ptr->me_value, out_key, out_value) < 0) { goto try_locked; } } @@ -5379,8 +5379,8 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self, if (i >= n) goto fail; - if (!acquire_key_value(&entry_ptr->me_key, value, - &entry_ptr->me_value, out_key, out_value)) { + if (acquire_key_value(&entry_ptr->me_key, value, + &entry_ptr->me_value, out_key, out_value) < 0) { goto try_locked; } }