Increase performance in unsafe_object_compare by not using Py_DECREF() for immortal res_obj

[benediktjohannes]

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

Linked PRs

• gh-145044: Increase performance in unsafe_object_compare by not using Py_DECREF() for immortal res_obj #145045