From eda29bc3d4717328286233529f5a88259a60e298 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sat, 2 Mar 2024 15:07:38 +0900 Subject: [PATCH 1/7] gh-112087: Update list_get_item_ref to optimistically avoid locking --- Objects/listobject.c | 60 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/Objects/listobject.c b/Objects/listobject.c index 79d61878c381bc..ce58ca7ca45a11 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -263,6 +263,65 @@ PyList_GetItemRef(PyObject *op, Py_ssize_t i) return Py_NewRef(PyList_GET_ITEM(op, i)); } +#ifdef Py_GIL_DISABLED + +static PyObject * +list_item_impl(PyListObject *self, Py_ssize_t idx, PyObject *dead) +{ + PyObject *item = NULL; + Py_BEGIN_CRITICAL_SECTION(self); + if (!_PyObject_GC_IS_SHARED(self)) { + _PyObject_GC_SET_SHARED(self); + } + Py_ssize_t size = Py_SIZE(self); + if (!valid_index(idx, size)) { + goto exit; + } + item = Py_NewRef(self->ob_item[idx]); +exit: + Py_END_CRITICAL_SECTION(); + return item; +} + +static inline PyObject* +list_get_item_ref(PyListObject *op, Py_ssize_t i) +{ + if (!_Py_IsOwnedByCurrentThread((PyObject *)op) && !_PyObject_GC_IS_SHARED(op)) { + return list_item_impl(op, i, NULL); + } + // Need atomic operation for the getting size. + Py_ssize_t size = _Py_atomic_load_ssize_relaxed(&_PyVarObject_CAST(op)->ob_size); + if (!valid_index(i, size)) { + return NULL; + } + PyObject **ob_item = _Py_atomic_load_ptr(&op->ob_item); + if (ob_item == NULL) { + return NULL; + } + Py_ssize_t cap = _Py_atomic_load_ssize_relaxed(&op->allocated); + if (!valid_index(i, cap)) { + return NULL; + } + PyObject *item = _Py_atomic_load_ptr(&ob_item[i]); + if (mi_unlikely(!item)) { + return list_item_impl(op, i, NULL); + } + if (mi_likely(_Py_TryIncrefFast(item))) { + goto compare_ob_item; + } + if (!_Py_TryIncRefShared(item)) { + return list_item_impl(op, i, item); + } + if (mi_unlikely(item != _Py_atomic_load_ptr(&ob_item[i]))) { + return list_item_impl(op, i, item); + } +compare_ob_item: + if (mi_unlikely(ob_item != _Py_atomic_load_ptr(&op->ob_item))) { + return list_item_impl(op, i, item); + } + return item; +} +#else static inline PyObject* list_get_item_ref(PyListObject *op, Py_ssize_t i) { @@ -271,6 +330,7 @@ list_get_item_ref(PyListObject *op, Py_ssize_t i) } return Py_NewRef(PyList_GET_ITEM(op, i)); } +#endif int PyList_SetItem(PyObject *op, Py_ssize_t i, From 9071dd4778241eff118e40fff749c7a4296aa0c6 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Tue, 5 Mar 2024 22:38:19 +0900 Subject: [PATCH 2/7] Update --- Objects/listobject.c | 61 ++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index ce58ca7ca45a11..02efcffb8f1193 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -233,36 +233,6 @@ valid_index(Py_ssize_t i, Py_ssize_t limit) return (size_t) i < (size_t) limit; } -PyObject * -PyList_GetItem(PyObject *op, Py_ssize_t i) -{ - if (!PyList_Check(op)) { - PyErr_BadInternalCall(); - return NULL; - } - if (!valid_index(i, Py_SIZE(op))) { - _Py_DECLARE_STR(list_err, "list index out of range"); - PyErr_SetObject(PyExc_IndexError, &_Py_STR(list_err)); - return NULL; - } - return ((PyListObject *)op) -> ob_item[i]; -} - -PyObject * -PyList_GetItemRef(PyObject *op, Py_ssize_t i) -{ - if (!PyList_Check(op)) { - PyErr_SetString(PyExc_TypeError, "expected a list"); - return NULL; - } - if (!valid_index(i, Py_SIZE(op))) { - _Py_DECLARE_STR(list_err, "list index out of range"); - PyErr_SetObject(PyExc_IndexError, &_Py_STR(list_err)); - return NULL; - } - return Py_NewRef(PyList_GET_ITEM(op, i)); -} - #ifdef Py_GIL_DISABLED static PyObject * @@ -332,6 +302,37 @@ list_get_item_ref(PyListObject *op, Py_ssize_t i) } #endif +PyObject * +PyList_GetItem(PyObject *op, Py_ssize_t i) +{ + if (!PyList_Check(op)) { + PyErr_BadInternalCall(); + return NULL; + } + if (!valid_index(i, Py_SIZE(op))) { + _Py_DECLARE_STR(list_err, "list index out of range"); + PyErr_SetObject(PyExc_IndexError, &_Py_STR(list_err)); + return NULL; + } + return ((PyListObject *)op) -> ob_item[i]; +} + +PyObject * +PyList_GetItemRef(PyObject *op, Py_ssize_t i) +{ + if (!PyList_Check(op)) { + PyErr_SetString(PyExc_TypeError, "expected a list"); + return NULL; + } + PyObject *item = list_get_item_ref((PyListObject *)op, i); + if (item == NULL) { + _Py_DECLARE_STR(list_err, "list index out of range"); + PyErr_SetObject(PyExc_IndexError, &_Py_STR(list_err)); + return NULL; + } + return item; +} + int PyList_SetItem(PyObject *op, Py_ssize_t i, PyObject *newitem) From f0999f874cb15c1227662a4a9bf3a69561af7234 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 6 Mar 2024 00:25:15 +0900 Subject: [PATCH 3/7] Address code review partially --- Objects/listobject.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 02efcffb8f1193..4846f9601d45f9 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -236,7 +236,7 @@ valid_index(Py_ssize_t i, Py_ssize_t limit) #ifdef Py_GIL_DISABLED static PyObject * -list_item_impl(PyListObject *self, Py_ssize_t idx, PyObject *dead) +list_item_impl(PyListObject *self, Py_ssize_t idx) { PyObject *item = NULL; Py_BEGIN_CRITICAL_SECTION(self); @@ -257,10 +257,10 @@ static inline PyObject* list_get_item_ref(PyListObject *op, Py_ssize_t i) { if (!_Py_IsOwnedByCurrentThread((PyObject *)op) && !_PyObject_GC_IS_SHARED(op)) { - return list_item_impl(op, i, NULL); + return list_item_impl(op, i); } // Need atomic operation for the getting size. - Py_ssize_t size = _Py_atomic_load_ssize_relaxed(&_PyVarObject_CAST(op)->ob_size); + Py_ssize_t size = PyList_GET_SIZE(op); if (!valid_index(i, size)) { return NULL; } @@ -273,21 +273,21 @@ list_get_item_ref(PyListObject *op, Py_ssize_t i) return NULL; } PyObject *item = _Py_atomic_load_ptr(&ob_item[i]); - if (mi_unlikely(!item)) { - return list_item_impl(op, i, NULL); + if (!item) { + return list_item_impl(op, i); } - if (mi_likely(_Py_TryIncrefFast(item))) { + if (_Py_TryIncrefFast(item)) { goto compare_ob_item; } if (!_Py_TryIncRefShared(item)) { - return list_item_impl(op, i, item); + return list_item_impl(op, i); } - if (mi_unlikely(item != _Py_atomic_load_ptr(&ob_item[i]))) { - return list_item_impl(op, i, item); + if (item != _Py_atomic_load_ptr(&ob_item[i])) { + return list_item_impl(op, i); } compare_ob_item: - if (mi_unlikely(ob_item != _Py_atomic_load_ptr(&op->ob_item))) { - return list_item_impl(op, i, item); + if (ob_item != _Py_atomic_load_ptr(&op->ob_item)) { + return list_item_impl(op, i); } return item; } From d5b7723e8d532549d5c2156beee7c789536a4dc3 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 6 Mar 2024 00:33:50 +0900 Subject: [PATCH 4/7] Address code review --- Objects/listobject.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Objects/listobject.c b/Objects/listobject.c index 4846f9601d45f9..5c30a1ab727340 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -283,10 +283,12 @@ list_get_item_ref(PyListObject *op, Py_ssize_t i) return list_item_impl(op, i); } if (item != _Py_atomic_load_ptr(&ob_item[i])) { + Py_DECREF(item); return list_item_impl(op, i); } compare_ob_item: if (ob_item != _Py_atomic_load_ptr(&op->ob_item)) { + Py_DECREF(item); return list_item_impl(op, i); } return item; From f03efb2ebb82794f4113f878c5d22669e06a11ec Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 6 Mar 2024 05:01:51 +0900 Subject: [PATCH 5/7] Update Objects/listobject.c Co-authored-by: Sam Gross --- Objects/listobject.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 5c30a1ab727340..10077aaf2f13b2 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -272,18 +272,8 @@ list_get_item_ref(PyListObject *op, Py_ssize_t i) if (!valid_index(i, cap)) { return NULL; } - PyObject *item = _Py_atomic_load_ptr(&ob_item[i]); - if (!item) { - return list_item_impl(op, i); - } - if (_Py_TryIncrefFast(item)) { - goto compare_ob_item; - } - if (!_Py_TryIncRefShared(item)) { - return list_item_impl(op, i); - } - if (item != _Py_atomic_load_ptr(&ob_item[i])) { - Py_DECREF(item); + PyObject *item = _Py_TryXGetRef(&ob_item[i]); + if (item == NULL) { return list_item_impl(op, i); } compare_ob_item: From d5d39ecef936d3b9cd2cb896ddeb9389c0db77b6 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 6 Mar 2024 05:04:11 +0900 Subject: [PATCH 6/7] fix --- Objects/listobject.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 10077aaf2f13b2..d89ab9042a2f55 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -276,11 +276,6 @@ list_get_item_ref(PyListObject *op, Py_ssize_t i) if (item == NULL) { return list_item_impl(op, i); } -compare_ob_item: - if (ob_item != _Py_atomic_load_ptr(&op->ob_item)) { - Py_DECREF(item); - return list_item_impl(op, i); - } return item; } #else From 2dc7762bd35517d77566ba5bd9db1e3f8ceb01fd Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 6 Mar 2024 05:38:17 +0900 Subject: [PATCH 7/7] Add assertion --- Objects/listobject.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Objects/listobject.c b/Objects/listobject.c index d89ab9042a2f55..e013383a712bc8 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -269,6 +269,7 @@ list_get_item_ref(PyListObject *op, Py_ssize_t i) return NULL; } Py_ssize_t cap = _Py_atomic_load_ssize_relaxed(&op->allocated); + assert(cap != -1 && cap >= size); if (!valid_index(i, cap)) { return NULL; }