Increase performance in unsafe_object_compare by not using Py_DECREF() for immortal res_obj #145044
Description
Feature or enhancement
Proposal:
In Objects/listobject.c we currently use Py_DECREF(res_obj); in the function unsafe_object_compare() for both cases if (PyBool_Check(res_obj)) and else (so not directly inside these, but underneath which means that it's triggered in both cases because there's no early return or something like this) while using Py_DECREF(res_obj); does not have any impact if res_obj is immortal which is the case if PyBool_Check(res_obj) returns true. This has measurable performance overhead (as measured in my microbenchmark, see PR # (the correct hashtag will be coming as soon as I've opened the PR, but I've already benchmarked this a couple of days ago since I'm working on this change and the benchmarking this)) and should be no problem to remove in my opinion (also this /* Note that we can't assert res == PyObject_RichCompareBool(v, w, Py_LT); because of evil compare functions like this: lambda a, b: int(random.random() * 3) - 1) (which is actually in test_sort.py) */ comment should not be any problem for the change in my opinion, but please correct me if I'm mistaken anywhere).
Current code:
static int
unsafe_object_compare(PyObject *v, PyObject *w, MergeState *ms)
{
PyObject *res_obj; int res;
/* No assumptions, because we check first: */
if (Py_TYPE(v)->tp_richcompare != ms->key_richcompare)
return PyObject_RichCompareBool(v, w, Py_LT);
assert(ms->key_richcompare != NULL);
res_obj = (*(ms->key_richcompare))(v, w, Py_LT);
if (res_obj == Py_NotImplemented) {
Py_DECREF(res_obj);
return PyObject_RichCompareBool(v, w, Py_LT);
}
if (res_obj == NULL)
return -1;
if (PyBool_Check(res_obj)) {
res = (res_obj == Py_True);
}
else {
res = PyObject_IsTrue(res_obj);
}
Py_DECREF(res_obj);
/* Note that we can't assert
* res == PyObject_RichCompareBool(v, w, Py_LT);
* because of evil compare functions like this:
* lambda a, b: int(random.random() * 3) - 1)
* (which is actually in test_sort.py) */
return res;
}
Suggested change (move Py_DECREF(res_obj); inside the else part because it should only be necessary in this case):
static int
unsafe_object_compare(PyObject *v, PyObject *w, MergeState *ms)
{
PyObject *res_obj; int res;
/* No assumptions, because we check first: */
if (Py_TYPE(v)->tp_richcompare != ms->key_richcompare)
return PyObject_RichCompareBool(v, w, Py_LT);
assert(ms->key_richcompare != NULL);
res_obj = (*(ms->key_richcompare))(v, w, Py_LT);
if (res_obj == Py_NotImplemented) {
Py_DECREF(res_obj);
return PyObject_RichCompareBool(v, w, Py_LT);
}
if (res_obj == NULL)
return -1;
if (PyBool_Check(res_obj)) {
res = (res_obj == Py_True);
}
else {
res = PyObject_IsTrue(res_obj);
Py_DECREF(res_obj);
}
/* Note that we can't assert
* res == PyObject_RichCompareBool(v, w, Py_LT);
* because of evil compare functions like this:
* lambda a, b: int(random.random() * 3) - 1)
* (which is actually in test_sort.py) */
return res;
}
Has this already been discussed elsewhere?
This is a minor feature, which does not need previous discussion elsewhere
Links to previous discussion of this feature:
No response