bpo-35059: Convert Py_INCREF() to static inline function

[vstinner]

• Convert Py_INCREF(), Py_DECREF(), _Py_NewReference() and
  _Py_ForgetReference() macros to static inline functions.
• Remove _Py_CHECK_REFCNT(): code moved into Py_DECREF().
• Add an unit test on Py_DECREF() to make sure that
  _Py_NegativeRefcount() reports the correct filename.

https://bugs.python.org/issue35059

[methane]

Would you convert _Py_Dealloc() to static inline function and remove _Py_COUNT_ALLOCS_COMMA?

[vstinner]

Would you convert _Py_Dealloc() to static inline function and remove _Py_COUNT_ALLOCS_COMMA?

My long term plan is to convert most macros to static inline function, but for this PR, I prefer to restrict the number of converted macros.

Ok for that once since it's used directly by _Py_DECREF() ;-)

[vstinner]

I reverted _Py_Dealloc() change: it's more complex than what I expected. See code in object.c:

#ifndef Py_TRACE_REFS /* For Py_LIMITED_API, we need an out-of-line version of _Py_Dealloc. Define this here, so we can undefine the macro. */ #undef _Py_Dealloc void _Py_Dealloc(PyObject *); void _Py_Dealloc(PyObject *op){_Py_INC_TPFREES(op) _Py_COUNT_ALLOCS_COMMA (*Py_TYPE(op)->tp_dealloc)(op)} #endif 

I'm not sure how to handle this case. This PR is already complex enough. I changed my mind, I don't think that _Py_Dealloc() should be changed in the same PR. I will do it in a following PR.

[vstinner]

Oh. I merged master into my PR and now the test fails on Windows :-(

"c:\projects\cpython\include\object.h(774): error C2065: '_Py_tracemalloc_config': undeclared identifier"

I wrote PR #10091 to try to fix the issue.

[vstinner]

Oh, I had to move _PyTraceMalloc_NewReference() inside object.h... Converting a macro into a proper function makes "dependencies" issues more obvious and requires to fix the code :-)

[vstinner]

This PR became too big: I created PR #10093 and PR #10094 to push simpler changed and make this PR shorter (really focus on Py_INCREF).

[nascheme]

Hello Victor,
I think the idea of slowly replacing complicated macros with inline functions is a good idea. The performance will be the same but the code will become easier to maintain. We need to do it carefully, as I'm sure you are already aware. Keeping the same type casting behavior can be a bit tricky.

Your changes look to be missing the "extern" declarations. Doing that will provide the compiler the knowledge of where to put the non-inline version of the function. E.g. if you do it, you can take the address of a function that is declared inline (at least, it works for me on GCC).

It is simple to do. E.g. in object.c put:
extern inline void _Py_INCREF(PyObject *op);

See https://www.greenend.org.uk/rjk/tech/inline.html. I think we should use strategy number 3 (if it works with all the compilers we care about).

[nascheme]

Thinking about this some more, if we can't rely on compilers to implement the C99 behavior, I'm not sure this project of changing macros to inline functions is worth doing. Providing an implementation for compilers that can't handle it will be ugly. It would be like strategy number 4 in the link above. Too ugly IMHO and we should just keep the macros.

If that does turn out to be the case, I'm sad. C99 was adopted as an ANSI standard in 2000. You would think 18 years would be enough time to implement it.

[vstinner]

Your changes look to be missing the "extern" declarations. Doing that will provide the compiler the knowledge of where to put the non-inline version of the function. E.g. if you do it, you can take the address of a function that is declared inline (at least, it works for me on GCC).

(Currently, you cannot get the address of Py_INCREF(), since it's a macro. So nobody does that.)

I'm using "static inline" but attribute((always_inline)) for GCC and clang and __force_inline for MSVC. I had to tune MSVC options to also inline in debug mode: PR #10094.

If the function is not inline, it's fine, it doesn't break the ABI: https://bugs.python.org/issue35059#msg328426

IMHO the main drawback is the function is not inlined is that the machine code of the function can be duplicated if I understood properly.

In my experience, Py_INCREF() is now always compiled, in release and debug mode: https://bugs.python.org/issue35059#msg328421

Moreover, I chose "static inline" because it is now required by the PEP 7 :-) https://www.python.org/dev/peps/pep-0007/#c-dialect

And it's already used in pydtrace.h (I already replaced "static inline" with my new Py_STATIC_INLINE() macro.)

Thinking about this some more, if we can't rely on compilers to implement the C99 behavior (...)

Python 3.6 and newer requires a C compiler which supports "static inline" (PEP 7, again).

[nascheme]

(Currently, you cannot get the address of Py_INCREF(), since it's a macro. So nobody does that.)

Sure but I was thinking of the case of making some other functions inline, e.g. _Py_<something> when we would like the function to be inline and we don't allow it to be used inside extension modules. In that case, you might want it to be possible to take the address of the function.

IMHO the main drawback is the function is not inlined is that the machine code of the function can be duplicated if I understood properly.

I am far from a C compiler expert but it is my understanding that this is the main downside of the "static inline" style (GCC style?). If you use just "inline" and then provide the "extern inline" declaration in the .c unit, you get a single non-inlined version.

Moreover, I chose "static inline" because it is now required by the PEP 7 :-) https://www.python.org
/dev/peps/pep-0007/#c-dialect

If that's the best way, fine. I wonder though if we should change PEP 7 to recommend the C99 style ("inline" in header, "extern inline" in .c unit). I like it better because you avoid the multiple copies problem.

A downside to the C99 style is that the compiler is free to not inline the function if it decides it is too large (or whatever). That could be a pro or con I guess, depending on what you are trying to do.

[vstinner]

I rewrote my PR to no longer use Py_STATIC_INLINE(), but use directly "static inline", as requested by @benjaminp: see https://bugs.python.org/issue35059#msg328746