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

gh-112087: Update list_get_item_ref to optimistically avoid locking #116353

Merged
merged 7 commits into from
Mar 5, 2024
Merged

gh-112087: Update list_get_item_ref to optimistically avoid locking #116353

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eda29bc
gh-112087: Update list_get_item_ref to optimistically avoid locking
corona10 Mar 2, 2024
9071dd4
Update
corona10 Mar 5, 2024
f0999f8
Address code review partially
corona10 Mar 5, 2024
d5b7723
Address code review
corona10 Mar 5, 2024
f03efb2
Update Objects/listobject.c
corona10 Mar 5, 2024
d5d39ec
fix
corona10 Mar 5, 2024
2dc7762
Add assertion
corona10 Mar 5, 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
85 changes: 74 additions & 11 deletions Objects/listobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,77 @@ valid_index(Py_ssize_t i, Py_ssize_t limit)
return (size_t) i < (size_t) limit;
}

#ifdef Py_GIL_DISABLED

static PyObject *
list_item_impl(PyListObject *self, Py_ssize_t idx)
{
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);
}
// Need atomic operation for the getting size.
Py_ssize_t size = PyList_GET_SIZE(op);
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is okay for now, but we need to store the capacity at the start of the ob_item allocation in the free-threaded build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need to create a issue for tracking this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's necessary for thread-safety so I think it's covered by #112087

Copy link
Member Author

@corona10 corona10 Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, we already store the op->allocated at the ob_item allocation.
So adding assertion will be enough?
assert(cap != -1 && cap >= size);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently store it in the PyListObject. We also need to store it in the same memory allocation as the ob_item array, like a pre-header.

Here are some pointers to the relevant code in nogil-3.12:

The problem with the current code is that the list may be resized concurrently with the access. The bounds check may be stale. Putting the value of "allocated" as an immutable field avoids this problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I got it. I understood what you want to say. Let's handle it at a separate PR.

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);
return list_item_impl(op, i);
}
corona10 marked this conversation as resolved.
Show resolved Hide resolved
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
static inline PyObject*
list_get_item_ref(PyListObject *op, Py_ssize_t i)
{
if (!valid_index(i, Py_SIZE(op))) {
return NULL;
}
return Py_NewRef(PyList_GET_ITEM(op, i));
}
#endif

PyObject *
PyList_GetItem(PyObject *op, Py_ssize_t i)
{
Expand All @@ -255,21 +326,13 @@ PyList_GetItemRef(PyObject *op, Py_ssize_t i)
PyErr_SetString(PyExc_TypeError, "expected a list");
return NULL;
}
if (!valid_index(i, Py_SIZE(op))) {
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 Py_NewRef(PyList_GET_ITEM(op, i));
}

static inline PyObject*
list_get_item_ref(PyListObject *op, Py_ssize_t i)
{
if (!valid_index(i, Py_SIZE(op))) {
return NULL;
}
return Py_NewRef(PyList_GET_ITEM(op, i));
return item;
}

int
Expand Down
Loading