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

Micro-optimize list index range checks #9784

Merged
merged 2 commits into from
Oct 11, 2018
Merged

Micro-optimize list index range checks #9784

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d152678
Optimize list_item
rhettinger Oct 10, 2018
65b589b
Convert the other range checks as well
rhettinger Oct 10, 2018
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
23 changes: 18 additions & 5 deletions Objects/listobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,19 @@ PyList_Size(PyObject *op)
return Py_SIZE(op);
}

static inline int
valid_index(Py_ssize_t i, Py_ssize_t limit)
Copy link
Member

Choose a reason for hiding this comment

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

Why not just define this as taking two size_t parameters instead of doing the casting below. The casts then happen implicitly at all call sites.

{
/* The cast to size_t lets us use just a single comparison
to check whether i is in the range: 0 <= i < limit.

See: Section 14.2 "Bounds Checking" in the Agner Fog
optimization manual found at:
https://www.agner.org/optimize/optimizing_cpp.pdf
*/
return (size_t) i < (size_t) limit;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that the behaviour is well defined in C. I fear that it's Undefined Behaviour. @benjaminp @gpshead: What do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

If it's well defined, why should we hack such micro optimization? Why compilers would not implement the optimization themself?

Copy link
Contributor

@sir-sigurd sir-sigurd Oct 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think because they don't know that Py_SIZE(op) is non-negative.

Copy link
Member

Choose a reason for hiding this comment

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

It is well defined. It is used for example in the STL implementations.

But there was not found any difference in microbenchmark results on 64-bit platforms in previous discussion in bpo-28397.

}

static PyObject *indexerr = NULL;

PyObject *
Expand All @@ -217,7 +230,7 @@ PyList_GetItem(PyObject *op, Py_ssize_t i)
PyErr_BadInternalCall();
return NULL;
}
if (i < 0 || i >= Py_SIZE(op)) {
if (!valid_index(i, Py_SIZE(op))) {
if (indexerr == NULL) {
indexerr = PyUnicode_FromString(
"list index out of range");
Expand All @@ -240,7 +253,7 @@ PyList_SetItem(PyObject *op, Py_ssize_t i,
PyErr_BadInternalCall();
return -1;
}
if (i < 0 || i >= Py_SIZE(op)) {
if (!valid_index(i, Py_SIZE(op))) {
Py_XDECREF(newitem);
PyErr_SetString(PyExc_IndexError,
"list assignment index out of range");
Expand Down Expand Up @@ -426,7 +439,7 @@ list_contains(PyListObject *a, PyObject *el)
static PyObject *
list_item(PyListObject *a, Py_ssize_t i)
{
if (i < 0 || i >= Py_SIZE(a)) {
if (!valid_index(i, Py_SIZE(a))) {
if (indexerr == NULL) {
indexerr = PyUnicode_FromString(
"list index out of range");
Expand Down Expand Up @@ -749,7 +762,7 @@ list_inplace_repeat(PyListObject *self, Py_ssize_t n)
static int
list_ass_item(PyListObject *a, Py_ssize_t i, PyObject *v)
{
if (i < 0 || i >= Py_SIZE(a)) {
if (!valid_index(i, Py_SIZE(a))) {
PyErr_SetString(PyExc_IndexError,
"list assignment index out of range");
return -1;
Expand Down Expand Up @@ -996,7 +1009,7 @@ list_pop_impl(PyListObject *self, Py_ssize_t index)
}
if (index < 0)
index += Py_SIZE(self);
if (index < 0 || index >= Py_SIZE(self)) {
if (!valid_index(index, Py_SIZE(self))) {
PyErr_SetString(PyExc_IndexError, "pop index out of range");
return NULL;
}
Expand Down