gh-144822: Remove an indirect double check in codegen.c through co == NULL and Py_XDECREF(co);

[benediktjohannes]

Description of the problem

In static int codegen_function_body(compiler *c, stmt_ty s, int is_async, Py_ssize_t funcflags, int firstlineno) we use a check whether co == NULL in

PyCodeObject *co = _PyCompile_OptimizeAndAssemble(c, 1);
_PyCompile_ExitScope(c);
if (co == NULL) {
    Py_XDECREF(co);
    return ERROR;
}

and then use Py_XDECREF(co); which always returns a check failure in the case co == NULL which we already checked previously and for this reason never calls Py_DECREF(co); (which would anyway not work because it would cause errors) before we return the ERROR. Because of this, that condition check (which is triggered again) is unnecessary and just decreases performance slightly.

Changes made

I've removed the unnecessary and performance consuming line for slightly increased performance and code readability.

Additional information

This was not tested (and also not performance tested), but seems obvious to me from looking at the code that this is just an unnecessary statement.

Contributed by Benedikt Johannes.

• Issue: Remove an indirect double check in codegen.c through co == NULL and Py_XDECREF(co); #144822

[eendebakpt]

Without benchmarks the news entry is a bit misleading and I think we could do without it. The code change itself is good.

[kumaraditya303]

There is no perf benefit because it is on the error path, please remove the news entry.

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[benediktjohannes]

Hi, thanks for the reviews! I agree with your points while I shortly want to mention why I thought of a potential (slightly) improvement in performance (and I still think that this is true, but I'm not quite sure). Although this is in an error path, this does not mean that it does not improve performance in my opinion because in this case just the telling of the fact that there's an error is faster. Now one might say that this won't bring any profit while I think that there could be some (edge) cases in which you for example have live code testing of different inputs for specific tests whether code is renderable or not in some cases which then might (in very edgy cases) lead to performance improvements if these are hot paths, but I agree that this is very edgy and maybe only theoretically, so I'll remove the news entry.

[benediktjohannes]

@kumaraditya303 should I remove the whole news entry or only the "performance" wording and change it to something like "removed double check" or something like this?

[benediktjohannes]

I've now changed the news entry, but if you want me to completely remove if, please let me know

[benediktjohannes]

I have made the requested changes; please review again

[bedevere-app]

Thanks for making the requested changes!

@kumaraditya303: please review the changes made to this pull request.

[kumaraditya303]

Yes, remove it completely.

[benediktjohannes]

Ok 👍

[benediktjohannes]

It's deleted @kumaraditya303

[benediktjohannes]

Ah, only maintainers can add skip news label @kumaraditya303

[benediktjohannes]

Thank you @StanFromIreland