Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-112075: Lock when inserting into shared keys #117824

Closed
wants to merge 2 commits into from
Closed

gh-112075: Lock when inserting into shared keys #117824

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d270d2
gh-112075: Lock when inserting into shared keys
colesbury Apr 8, 2024
2e5797e
Fix return type
colesbury Apr 15, 2024
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
228 changes: 104 additions & 124 deletions Objects/dictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1606,31 +1606,6 @@ insertion_resize(PyInterpreterState *interp, PyDictObject *mp, int unicode)
return dictresize(interp, mp, calculate_log2_keysize(GROWTH_RATE(mp)), unicode);
}

static Py_ssize_t
insert_into_splitdictkeys(PyDictKeysObject *keys, PyObject *name, Py_hash_t hash)
{
assert(PyUnicode_CheckExact(name));
ASSERT_KEYS_LOCKED(keys);

Py_ssize_t ix = unicodekeys_lookup_unicode(keys, name, hash);
if (ix == DKIX_EMPTY) {
if (keys->dk_usable <= 0) {
return DKIX_EMPTY;
}
/* Insert into new slot. */
keys->dk_version = 0;
Py_ssize_t hashpos = find_empty_slot(keys, hash);
ix = keys->dk_nentries;
PyDictUnicodeEntry *ep = &DK_UNICODE_ENTRIES(keys)[ix];
dictkeys_set_index(keys, hashpos, ix);
assert(ep->me_key == NULL);
ep->me_key = Py_NewRef(name);
split_keys_entry_added(keys);
}
assert (ix < SHARED_KEYS_MAX_SIZE);
return ix;
}

static inline int
insert_combined_dict(PyInterpreterState *interp, PyDictObject *mp,
Py_hash_t hash, PyObject *key, PyObject *value)
Expand Down Expand Up @@ -1664,39 +1639,56 @@ insert_combined_dict(PyInterpreterState *interp, PyDictObject *mp,
return 0;
}

static int
insert_split_dict(PyInterpreterState *interp, PyDictObject *mp,
Py_hash_t hash, PyObject *key, PyObject *value)
static Py_ssize_t
insert_split_key(PyDictKeysObject *keys, PyObject *key, Py_hash_t hash)
{
PyDictKeysObject *keys = mp->ma_keys;
LOCK_KEYS(keys);
if (keys->dk_usable <= 0) {
/* Need to resize. */
UNLOCK_KEYS(keys);
int ins = insertion_resize(interp, mp, 1);
if (ins < 0) {
return -1;
}
assert(!_PyDict_HasSplitTable(mp));
return insert_combined_dict(interp, mp, hash, key, value);
}

Py_ssize_t hashpos = find_empty_slot(keys, hash);
dictkeys_set_index(keys, hashpos, keys->dk_nentries);

PyDictUnicodeEntry *ep;
ep = &DK_UNICODE_ENTRIES(keys)[keys->dk_nentries];
STORE_SHARED_KEY(ep->me_key, key);
assert(PyUnicode_CheckExact(key));
Py_ssize_t ix;

Py_ssize_t index = keys->dk_nentries;
_PyDictValues_AddToInsertionOrder(mp->ma_values, index);
assert (mp->ma_values->values[index] == NULL);
STORE_SPLIT_VALUE(mp, index, value);
#ifdef Py_GIL_DISABLED
ix = unicodekeys_lookup_unicode_threadsafe(keys, key, hash);
if (ix >= 0) {
return ix;
}
#endif

split_keys_entry_added(keys);
assert(keys->dk_usable >= 0);
LOCK_KEYS(keys);
ix = unicodekeys_lookup_unicode(keys, key, hash);
if (ix == DKIX_EMPTY && keys->dk_usable > 0) {
// Insert into new slot
keys->dk_version = 0;
Py_ssize_t hashpos = find_empty_slot(keys, hash);
ix = keys->dk_nentries;
dictkeys_set_index(keys, hashpos, ix);
PyDictUnicodeEntry *ep = &DK_UNICODE_ENTRIES(keys)[ix];
STORE_SHARED_KEY(ep->me_key, Py_NewRef(key));
split_keys_entry_added(keys);
}
assert (ix < SHARED_KEYS_MAX_SIZE);
UNLOCK_KEYS(keys);
return 0;
return ix;
}

static void
insert_split_value(PyInterpreterState *interp, PyDictObject *mp, PyObject *key, PyObject *value, Py_ssize_t ix)
{
assert(PyUnicode_CheckExact(key));
MAINTAIN_TRACKING(mp, key, value);
PyObject *old_value = mp->ma_values->values[ix];
if (old_value == NULL) {
uint64_t new_version = _PyDict_NotifyEvent(interp, PyDict_EVENT_ADDED, mp, key, value);
STORE_SPLIT_VALUE(mp, ix, Py_NewRef(value));
_PyDictValues_AddToInsertionOrder(mp->ma_values, ix);
mp->ma_used++;
mp->ma_version_tag = new_version;
}
else {
uint64_t new_version = _PyDict_NotifyEvent(interp, PyDict_EVENT_MODIFIED, mp, key, value);
STORE_SPLIT_VALUE(mp, ix, Py_NewRef(value));
mp->ma_version_tag = new_version;
Py_DECREF(old_value);
}
ASSERT_CONSISTENT(mp);
}

/*
Expand All @@ -1719,6 +1711,21 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp,
assert(mp->ma_keys->dk_kind == DICT_KEYS_GENERAL);
}

if (mp->ma_keys->dk_kind == DICT_KEYS_SPLIT) {
Py_ssize_t ix = insert_split_key(mp->ma_keys, key, hash);
if (ix != DKIX_EMPTY) {
insert_split_value(interp, mp, key, value, ix);
Py_DECREF(key);
Py_DECREF(value);
return 0;
}

/* No space in shared keys. Resize and continue below. */
if (insertion_resize(interp, mp, 1) < 0) {
goto Fail;
}
}

Py_ssize_t ix = _Py_dict_lookup(mp, key, hash, &old_value);
if (ix == DKIX_ERROR)
goto Fail;
Expand All @@ -1731,17 +1738,9 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp,
/* Insert into new slot. */
mp->ma_keys->dk_version = 0;
assert(old_value == NULL);

if (!_PyDict_HasSplitTable(mp)) {
if (insert_combined_dict(interp, mp, hash, key, value) < 0) {
goto Fail;
}
}
else {
if (insert_split_dict(interp, mp, hash, key, value) < 0)
goto Fail;
if (insert_combined_dict(interp, mp, hash, key, value) < 0) {
goto Fail;
}

mp->ma_used++;
mp->ma_version_tag = new_version;
ASSERT_CONSISTENT(mp);
Expand All @@ -1751,21 +1750,12 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp,
if (old_value != value) {
uint64_t new_version = _PyDict_NotifyEvent(
interp, PyDict_EVENT_MODIFIED, mp, key, value);
if (_PyDict_HasSplitTable(mp)) {
mp->ma_values->values[ix] = value;
if (old_value == NULL) {
_PyDictValues_AddToInsertionOrder(mp->ma_values, ix);
mp->ma_used++;
}
assert(old_value != NULL);
if (DK_IS_UNICODE(mp->ma_keys)) {
DK_UNICODE_ENTRIES(mp->ma_keys)[ix].me_value = value;
}
else {
assert(old_value != NULL);
if (DK_IS_UNICODE(mp->ma_keys)) {
DK_UNICODE_ENTRIES(mp->ma_keys)[ix].me_value = value;
}
else {
DK_ENTRIES(mp->ma_keys)[ix].me_value = value;
}
DK_ENTRIES(mp->ma_keys)[ix].me_value = value;
}
mp->ma_version_tag = new_version;
}
Expand Down Expand Up @@ -4173,6 +4163,29 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu
}
}

if (mp->ma_keys->dk_kind == DICT_KEYS_SPLIT) {
Py_ssize_t ix = insert_split_key(mp->ma_keys, key, hash);
if (ix != DKIX_EMPTY) {
PyObject *value = mp->ma_values->values[ix];
int already_present = value != NULL;
if (!already_present) {
insert_split_value(interp, mp, key, default_value, ix);
value = default_value;
}
if (result) {
*result = incref_result ? Py_NewRef(value) : value;
}
return already_present;
}

/* No space in shared keys. Resize and continue below. */
if (insertion_resize(interp, mp, 1) < 0) {
goto error;
}
}

assert(!_PyDict_HasSplitTable(mp));

Py_ssize_t ix = _Py_dict_lookup(mp, key, hash, &value);
if (ix == DKIX_ERROR) {
if (result) {
Expand All @@ -4187,25 +4200,13 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu
mp->ma_keys->dk_version = 0;
value = default_value;

if (!_PyDict_HasSplitTable(mp)) {
if (insert_combined_dict(interp, mp, hash, Py_NewRef(key), Py_NewRef(value)) < 0) {
Py_DECREF(key);
Py_DECREF(value);
if (result) {
*result = NULL;
}
return -1;
}
}
else {
if (insert_split_dict(interp, mp, hash, Py_NewRef(key), Py_NewRef(value)) < 0) {
Py_DECREF(key);
Py_DECREF(value);
if (result) {
*result = NULL;
}
return -1;
if (insert_combined_dict(interp, mp, hash, Py_NewRef(key), Py_NewRef(value)) < 0) {
Py_DECREF(key);
Py_DECREF(value);
if (result) {
*result = NULL;
}
return -1;
}

MAINTAIN_TRACKING(mp, key, value);
Expand All @@ -4218,29 +4219,19 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu
}
return 0;
}
else if (value == NULL) {
uint64_t new_version = _PyDict_NotifyEvent(
interp, PyDict_EVENT_ADDED, mp, key, default_value);
value = default_value;
assert(_PyDict_HasSplitTable(mp));
assert(mp->ma_values->values[ix] == NULL);
MAINTAIN_TRACKING(mp, key, value);
mp->ma_values->values[ix] = Py_NewRef(value);
_PyDictValues_AddToInsertionOrder(mp->ma_values, ix);
mp->ma_used++;
mp->ma_version_tag = new_version;
ASSERT_CONSISTENT(mp);
if (result) {
*result = incref_result ? Py_NewRef(value) : value;
}
return 0;
}

assert(value != NULL);
ASSERT_CONSISTENT(mp);
if (result) {
*result = incref_result ? Py_NewRef(value) : value;
}
return 1;

error:
if (result) {
*result = NULL;
}
return -1;
}

int
Expand Down Expand Up @@ -6644,18 +6635,7 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values,
assert(hash != -1);
}

#ifdef Py_GIL_DISABLED
// Try a thread-safe lookup to see if the index is already allocated
ix = unicodekeys_lookup_unicode_threadsafe(keys, name, hash);
if (ix == DKIX_EMPTY || ix == DKIX_KEY_CHANGED) {
// Lock keys and do insert
LOCK_KEYS(keys);
ix = insert_into_splitdictkeys(keys, name, hash);
UNLOCK_KEYS(keys);
}
#else
ix = insert_into_splitdictkeys(keys, name, hash);
#endif
ix = insert_split_key(keys, name, hash);

#ifdef Py_STATS
if (ix == DKIX_EMPTY) {
Expand Down
Loading