Skip to content
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

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

Merged
merged 1 commit into from Oct 29, 2018

Conversation

@vstinner
Copy link
Member

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 requested review from benjaminp, gpshead and nascheme Oct 24, 2018

@vstinner vstinner requested a review from python/windows-team as a code owner Oct 25, 2018

@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

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

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

@vstinner

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

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 comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

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

@vstinner vstinner force-pushed the vstinner:incref branch 2 times, most recently from 296be90 to 6b7e06a Oct 25, 2018

@vstinner vstinner added the skip news label Oct 25, 2018

@nascheme

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

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 Oct 26, 2018

@nascheme

This comment has been minimized.

Copy link
Member

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.

bpo-35059: Convert Py_INCREF() to static inline function
* Convert Py_INCREF() and Py_DECREF() macros into static inline
  functions.
* Remove _Py_CHECK_REFCNT() macro: code moved into Py_DECREF().

@vstinner vstinner force-pushed the vstinner:incref branch from 6b7e06a to 8b994b9 Oct 28, 2018

@vstinner

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2018

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

5 checks passed

Azure Pipelines PR #20181028.64 succeeded
Details
bedevere/issue-number Issue number 35059 found
Details
bedevere/news "skip news" label found
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vstinner vstinner deleted the vstinner:incref branch Oct 29, 2018

yahya-abou-imran added a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
bpo-35059: Convert Py_INCREF() to static inline function (pythonGH-10079
)

* Convert Py_INCREF() and Py_DECREF() macros into static inline
  functions.
* Remove _Py_CHECK_REFCNT() macro: code moved into Py_DECREF().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.