git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Python-Dev] General concerns about C API changes


Le dim. 18 nov. 2018 ? 17:54, Stefan Behnel <stefan_ml at behnel.de> a ?crit :
> It's also slower to compile, given that function inlining happens at a much
> later point in the compiler pipeline than macro expansion. The C compiler
> won't even get to see macros in fact, whereas whether to inline a function
> or not is a dedicated decision during the optimisation phase based on
> metrics collected in earlier stages. For something as ubiquitous as
> Py_INCREF/Py_DECREF, it might even be visible in the compilation times.

I ran a benchmark: there is no significant slowdown (+4 seconds, 6%
slower, in the worst case).
https://bugs.python.org/issue35059#msg330316


> Now imagine that you have an inline function that executes several
> Py_INCREF/Py_DECREF call cycles, and the C compiler happens to slightly
> overestimate the weights of these two. Then it might end up deciding
> against inlining the function now, whereas it previously might have decided
> for it since it was able to see the exact source code expanded from the
> macros. I think that's what Raymond meant with his concerns regarding
> changing macros into inline functions. C compilers might be smart enough to
> always inline CPython's new inline functions themselves, but the style
> change can still have unexpected transitive impacts on code that uses them.

I ran the performance benchmark suite to compare C macros to static
inline functions: there is no significant impact on performance.
https://bugs.python.org/issue35059#msg330302


> I agree with Raymond that as long as there is no clear gain in this code
> churn, we should not underestimate the risk of degarding code on user side.

I don't understand how what you mean with "degarding code on user
side". If you are talking about performance, again, my changes have no
significant impact on performance (not on compilation time nor runtime
performance).


> "there is no clear gain in this code churn"

There are multiple advantages:

* Better development and debugging experience: tools understand
inlined functions much better than C macros: gdb, Linux perf, etc.

* Better API: arguments now have a type and the function has a return
type. In practice, some macros still cast their argument to PyObject*
to not introduce new compiler warnings in Python 3.8. For example,
even if Py_INCREF() is documented (*) as a function expecting
PyObject*, it accepts any pointer type (PyTupleObject*,
PyUnicodeObject*, etc.). Technically, it also accepts PyObject** which
is a bug, but that's a different story ;-)

* Much better code, just plain regular C. C macros are ugly: "do { ...
} while (0)" workaround, additional parenthesis around each argument,
strange "expr1, expr2" syntax of "macro expression" which returns a
value (inline function just uses regular "return" and ";" at the end
of instructions), strange indentation, etc.

* No more "macro pitfals":
https://gcc.gnu.org/onlinedocs/cpp/Macro-Pitfalls.html

* Local variables no longer need a magic name to avoid risk of name
conflict, and have a clearly defined scope. Py_DECREF() and
_Py_XINCREF() no longer need a local variable since it's argument
already has a clearly defined type: PyObject*. I introduced a new
variable in _Py_Dealloc() to fix a possible race condition.
Previously, the variable was probably avoided because it's tricky use
variables in macros.

* #ifdef can now be used inside the inline function: it makes the code
easier to understand.

* etc.


Are you aware that Python had macros like:

#define _Py_REF_DEBUG_COMMA ,
#define _Py_CHECK_REFCNT(OP) /* a semicolon */;

I let you judge the quality of this macro:

#define _Py_NewReference(op) (                          \
    _Py_INC_TPALLOCS(op) _Py_COUNT_ALLOCS_COMMA         \
    _Py_INC_REFTOTAL  _Py_REF_DEBUG_COMMA               \
    Py_REFCNT(op) = 1)

Is it an expression? Can it be used in "if (test)
_Py_NewReference(op);"? It doesn't use the "do { ... } while (0)"
protection against macro pitfals.

(*) Py_INCREF doc:
https://docs.python.org/dev/c-api/refcounting.html#c.Py_INCREF

Victor