Skip to content

gh-112087: Make list_repr and list_length to be thread-safe #114582

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

Merged
corona10 merged 3 commits into from
Jan 26, 2024
Merged

gh-112087: Make list_repr and list_length to be thread-safe #114582

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b8c0c7
gh-112087: Make list_repr and list_length to be thread-safe
corona10 Jan 26, 2024
7009cec
Address code review
corona10 Jan 26, 2024
4d0fe6a
nit
corona10 Jan 26, 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
29 changes: 20 additions & 9 deletions Objects/listobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,18 +383,11 @@ list_dealloc(PyObject *self)
}

static PyObject *
list_repr(PyObject *self)
list_repr_impl(PyListObject *v)
{
PyListObject *v = (PyListObject *)self;
Py_ssize_t i;
PyObject *s;
_PyUnicodeWriter writer;

if (Py_SIZE(v) == 0) {
return PyUnicode_FromString("[]");
}

i = Py_ReprEnter((PyObject*)v);
Py_ssize_t i = Py_ReprEnter((PyObject*)v);
if (i != 0) {
return i > 0 ? PyUnicode_FromString("[...]") : NULL;
}
Expand Down Expand Up @@ -439,10 +432,28 @@ list_repr(PyObject *self)
return NULL;
}

static PyObject *
list_repr(PyObject *self)
{
PyListObject *v = (PyListObject *)self;
PyObject *ret = NULL;
if (Py_SIZE(v) == 0) {
return PyUnicode_FromString("[]");
}
Py_BEGIN_CRITICAL_SECTION(v);
ret = list_repr_impl(v);
Py_END_CRITICAL_SECTION();
return ret;
}

static Py_ssize_t
list_length(PyObject *a)
{
#ifdef Py_GIL_DISABLED
return _Py_atomic_load_ssize_relaxed(&(_PyVarObject_CAST(a)->ob_size));
#else
return Py_SIZE(a);
Copy link
Copy Markdown
Member Author

@corona10 corona10 Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@encukou said that making Py_SIZE to be thread-safe for free-threaded build would be beneficial rather than implementing list_length with macro if possible :)

Copy link
Copy Markdown
Member

@encukou encukou Jan 26, 2024

Choose a reason for hiding this comment

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

It could. I don't know enough about the grand plan to know for sure :)

Copy link
Copy Markdown
Contributor

@colesbury colesbury Jan 26, 2024

Choose a reason for hiding this comment

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

I think pushing the atomic load down to Py_SIZE() is reasonable and will probably simplify call sites.

That was something I tried and then abandoned in nogil-3.9 because at the time the macro was also used as an l-value like Py_SIZE(ob) = 1. Fortunately, that's no longer a concern.

Copy link
Copy Markdown
Contributor

@colesbury colesbury Jan 26, 2024

Choose a reason for hiding this comment

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

Alternatively, we could just do the atomic load in PyList_GET_SIZE() and consistently use that within this file. I think I'd prefer that.

There are some loops that use Py_SIZE() on immutable objects that might slow down if we make Py_SIZE use an atomic load. For example, in codeobject.c:

cpython/Objects/codeobject.c

Lines 433 to 436 in 30b7b4f

while (entry_point < Py_SIZE(co) &&
_PyCode_CODE(co)[entry_point].op.code != RESUME) {
entry_point++;
}

The compiler will currently lift the Py_SIZE() load outside the loop. But it won't do that optimization with an atomic load.

It's hard to know if these sorts of things will make a difference overall, but it's often easier to avoid the potential performance issues in the first place than trying to find, benchmark, and fix the issues later.

Copy link
Copy Markdown
Contributor

@colesbury colesbury Jan 26, 2024

Choose a reason for hiding this comment

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

Sorry, my opinion keeps changing as I read through listobject.c. Changing all the Py_SIZE() calls to PyList_GET_SIZE() seems like it would be a lot of noise. To be honest, I think there are a lot of reasonable approaches, and whatever you decide to do is fine with me. Here is my current thinking:

  • Most of the Py_SIZE() calls are reads from functions that will be within critical sections in the future. Those are fine as is.
  • If we read ob_size outside of a critical section, it should use an atomic load. Either directly or indirectly, such as by calling list_length().

For list_repr, my inclination would be to push the zero-size check down into the critical section, but using an atomic load also seems fine.

Copy link
Copy Markdown
Contributor

@erlend-aasland erlend-aasland Jan 26, 2024

Choose a reason for hiding this comment

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

Fortunately, that's no longer a concern.

Thanks to @vstinner for fixing this! 👏

Copy link
Copy Markdown
Member Author

@corona10 corona10 Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm.. well, from the view of maintaining implementation detail as not changed as possible for the list object itself, updating PyList_GET_SIZE() to be an atomic operation is worth doing. It's good negotiation point I guess.

If we need to make Py_SIZE() as atomic operation, we can handle it later.

#endif
}

static int
Expand Down