Skip to content

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

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.

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

Merged
merged 1 commit into from
Oct 29, 2018
Merged

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

merged 1 commit into from
Oct 29, 2018

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 24, 2018

  • 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

@vstinner vstinner changed the title bpo-35059: Convert Py_INCREF() to function bpo-35059: Convert Py_INCREF() to static inline function Oct 25, 2018
@methane
Copy link
Member

methane commented Oct 25, 2018

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

@vstinner
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

nascheme commented Oct 25, 2018

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
Copy link
Member

nascheme commented Oct 25, 2018

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
Copy link
Member Author

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).

@vstinner vstinner requested a review from pitrou October 26, 2018 00:03
@nascheme
Copy link
Member

nascheme commented Oct 26, 2018

(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.

* Convert Py_INCREF() and Py_DECREF() macros into static inline
  functions.
* Remove _Py_CHECK_REFCNT() macro: code moved into Py_DECREF().
@vstinner
Copy link
Member Author

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

@vstinner vstinner merged commit 2aaf0c1 into python:master Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants