Convert Py_INCREF() and PyObject_INIT() to inlined functions

[vstinner]

BPO	35059
Nosy	@mdickinson, @vstinner, @benjaminp, @ericsnowcurrently, @serhiy-storchaka, @zooba, @srinivasreddy, @aaronchall, @pablogsal, @miss-islington
PRs	bpo-35059: Convert _Py_NewReference() to function #10076 bpo-35059: Convert PyObject_INIT() to function #10077 bpo-35059: Convert Py_INCREF() to static inline function #10079 bpo-35059: Add Py_STATIC_INLINE() macro #10093 bpo-35059 : Add /Ob1 flag when building pythoncore in debug mode #10094 Revert "bpo-35059 : Add /Ob1 flag when building pythoncore in debug mode" #10127 bpo-35059, libmpdec: Add missing EXTINLINE in mpdecimal.h #10128 [3.6] bpo-35059, libmpdec: Add missing EXTINLINE in mpdecimal.h (GH-10128) #10134 [3.7] bpo-35059, libmpdec: Add missing EXTINLINE in mpdecimal.h (GH-10128) #10135 bpo-35059: Remove Py_STATIC_INLINE() macro #10216 bpo-35059: Convert _Py_Dealloc() to static inline function #10223 bpo-35059: Convert Py_XINCREF() to static inline function #10224 WIP: bpo-35059: _PyThreadState_GET() checks that the GIL is hold #10278 bpo-35059: Enhance _PyObject_AssertFailed() #10642 bpo-35059: Enhance _PyObject_AssertFailed() #10642 bpo-35059: Enhance _PyObject_AssertFailed() #10642 bpo-35059: Convert _PyObject_GC_TRACK() to inline function #10643 bpo-35059: Convert _PyObject_GC_TRACK() to inline function #10643 bpo-35059: Convert _PyObject_GC_TRACK() to inline function #10643 bpo-35059: Add _PyObject_CAST() macro #10645 bpo-35059: Cleanup load_extension() of _pickle.c #10647 bpo-35059: Cleanup usage of Python macros #10648 [WIP] bpo-35059: Add assertion to _PyObject_CAST() #10649 bpo-35059: Cast void* to PyObject* #10650 WIP: bpo-35059: Replace inline with macros #10669 bpo-35059: Add NEWS entry #10671 bpo-35059: PyObject_INIT() casts to PyObject* #10674 bpo-35059: Enhance _PyObject_GC_TRACK() macros #20931
Files	2018-11-22_17-38-master-3bb183d7fb83-patch-10669.json.gz

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-11-23.15:26:57.489>
created_at = <Date 2018-10-24.14:31:56.305>
labels = ['interpreter-core', '3.8']
title = 'Convert Py_INCREF() and PyObject_INIT() to inlined functions'
updated_at = <Date 2021-09-03.13:27:37.824>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2021-09-03.13:27:37.824>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-11-23.15:26:57.489>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2018-10-24.14:31:56.305>
creator = 'vstinner'
dependencies = []
files = ['47943']
hgrepos = []
issue_num = 35059
keywords = ['patch']
message_count = 55.0
messages = ['328367', '328376', '328408', '328416', '328418', '328420', '328421', '328423', '328424', '328426', '328427', '328450', '328514', '328529', '328542', '328543', '328552', '328557', '328570', '328577', '328578', '328622', '328746', '328757', '328759', '328760', '328823', '328829', '328830', '328855', '328857', '328919', '329527', '329842', '330225', '330226', '330229', '330230', '330239', '330302', '330305', '330310', '330314', '330316', '330320', '330321', '330323', '330329', '330330', '330338', '330353', '330363', '330365', '371733', '400991']
nosy_count = 10.0
nosy_names = ['mark.dickinson', 'vstinner', 'benjamin.peterson', 'eric.snow', 'serhiy.storchaka', 'steve.dower', 'thatiparthy', 'Aaron Hall', 'pablogsal', 'miss-islington']
pr_nums = ['10076', '10077', '10079', '10093', '10094', '10127', '10128', '10134', '10135', '10216', '10223', '10224', '10278', '10642', '10642', '10642', '10643', '10643', '10643', '10645', '10647', '10648', '10649', '10650', '10669', '10671', '10674', '20931']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue35059'
versions = ['Python 3.8']

[vstinner]

CPython has been created in 1990. In 1990, it made sense to use C macros. But nowadays, inlined functions can be used instead:

"Python versions greater than or equal to 3.6 use C89 with several select C99 features: (...) static inline functions"
https://www.python.org/dev/peps/pep-0007/#c-dialect

I propose to convert 4 macros to inlined functions:

• PyObject_INIT(), PyObject_INIT_VAR()
• _Py_NewReference(), _Py_ForgetReference()

Advantages:

• Functions use regular C syntax
• No more corner cases ("traps") of macros
• Function arguments have a type

Drawbacks:

• Require a specific type can introduce compiler warnings if the caller doesn't pass the proper type (PyObject* or PyVarObject*). _Py_NewReference() and _Py_ForgetReference() seem to be properly used, but not PyObject_INIT() and PyObject_INIT_VAR().

The two attached PRs implements these changes.

[vstinner]

Context: I was unhappy with _Py_NewReference() macro implementation when I had to modify it to fix a bug, bpo-35053. That's why I would like to convert it to a static inline function.

[benjaminp]

Does this slow down debug builds at all? It probably will not end will if Py_INCREF is ever not inlined.

[vstinner]

I tested PR 10079 using gdb on Fedora 28 with GCC 8.1.1 to check if Py_INCREF/Py_DECREF functions are inlined.

I understand that "static inline void Py_INCREF()" is *not* inline by gcc -O0, but it *is* inlined using gcc -Og which is the *default* optimization level of ./configure --with-debug.

To develop on Python, I force -O0, because the compilation time matters more than runtime performance for me :-) Compilation on my laptop using MAKEFLAGS=-j9:

• -O0: 21s
• -0g: 36s (1.7x slower)

... But Py_INCREF/DECREF are always inlined, even with -O0, when using __attribute__((always_inline))! I will work on a change to use that.

== gcc -O0 ==

(gdb) disassemble Py_IncRef
Dump of assembler code for function Py_IncRef:
...
0x000000000047276a <+20>: cmpq $0x0,-0x8(%rbp)
0x000000000047276f <+25>: je 0x47277d <Py_IncRef+39>
0x0000000000472771 <+27>: mov -0x8(%rbp),%rax
0x0000000000472775 <+31>: mov %rax,%rdi
0x0000000000472778 <+34>: callq 0x472523 <_Py_INCREF>
0x000000000047277d <+39>: nop
0x000000000047277e <+40>: leaveq
0x000000000047277f <+41>: retq

(gdb) disassemble Py_DecRef
Dump of assembler code for function Py_DecRef:
...
0x0000000000472794 <+20>: cmpq $0x0,-0x8(%rbp)
0x0000000000472799 <+25>: je 0x4727b1 <Py_DecRef+49>
0x000000000047279b <+27>: mov -0x8(%rbp),%rax
0x000000000047279f <+31>: mov %rax,%rdx
0x00000000004727a2 <+34>: mov $0xe1,%esi
0x00000000004727a7 <+39>: mov $0x65c550,%edi
0x00000000004727ac <+44>: callq 0x472554 <_Py_DECREF>
0x00000000004727b1 <+49>: nop
0x00000000004727b2 <+50>: leaveq
0x00000000004727b3 <+51>: retq

== gcc -Og ==

(gdb) disassemble Py_IncRef
Dump of assembler code for function Py_IncRef:
0x0000000000462de2 <+0>: test %rdi,%rdi
0x0000000000462de5 <+3>: je 0x462dfb <Py_IncRef+25>
0x0000000000462de7 <+5>: addq $0x1,0x4bfc09(%rip) # 0x9229f8 <_Py_RefTotal>
0x0000000000462def <+13>: mov 0x10(%rdi),%rax
0x0000000000462df3 <+17>: add $0x1,%rax
0x0000000000462df7 <+21>: mov %rax,0x10(%rdi)
0x0000000000462dfb <+25>: retq

(gdb) disassemble Py_DecRef
Dump of assembler code for function Py_DecRef:
0x0000000000463b2e <+0>: test %rdi,%rdi
0x0000000000463b31 <+3>: je 0x463b6f <Py_DecRef+65>
0x0000000000463b33 <+5>: sub $0x8,%rsp
0x0000000000463b37 <+9>: subq $0x1,0x4beeb9(%rip) # 0x9229f8 <_Py_RefTotal>
0x0000000000463b3f <+17>: mov 0x10(%rdi),%rax
0x0000000000463b43 <+21>: sub $0x1,%rax
0x0000000000463b47 <+25>: mov %rax,0x10(%rdi)
0x0000000000463b4b <+29>: je 0x463b68 <Py_DecRef+58>
0x0000000000463b4d <+31>: js 0x463b54 <Py_DecRef+38>
0x0000000000463b4f <+33>: add $0x8,%rsp
0x0000000000463b53 <+37>: retq
0x0000000000463b54 <+38>: mov %rdi,%rdx
0x0000000000463b57 <+41>: mov $0xe1,%esi
0x0000000000463b5c <+46>: mov $0x5e1120,%edi
0x0000000000463b61 <+51>: callq 0x462da4 <_Py_NegativeRefcount>
0x0000000000463b66 <+56>: jmp 0x463b4f <Py_DecRef+33>
0x0000000000463b68 <+58>: callq 0x463b0c <_Py_Dealloc>
0x0000000000463b6d <+63>: jmp 0x463b4f <Py_DecRef+33>
0x0000000000463b6f <+65>: retq

[vstinner]

I modified PR 10079 to add a Py_STATIC_INLINE(TYPE) macro:

• Use __attribute__((always_inline)) with GCC and clang
• Use __forceinline with MSVC

Tests on Linux, example:

"./configure --with-pydebug CC=clang CFLAGS="-O0" && make clean && make platform"

• Linux, gcc -O0: inlined
• Linux, clang -O0: inlined

Test done on Fedora 28 with GCC 8.1.1 and clang 6.0.1.

[vstinner]

On Windows, a Debug build doesn't inline Py_INCREF/DECREF even if it uses __forceinline. I looked at the Py_IncRef() and Py_DecRef() assembly in Visual Studio using a breakpoint.

Using /Ob1, Py_INCREF/DECREF are inlined as expected. I set this option in the pythoncore project.

Do you think that I should modify the 38 other projects of the pcbuild solution to enable /Ob1 in debug build?

Documentations.

Inline Functions (C++):
https://docs.microsoft.com/en-us/cpp/cpp/inline-functions-cpp?view=vs-2017

-Od: disable optimization ("d" stands for Debug)
https://msdn.microsoft.com/en-us/library/aafb762y.aspx

/Ob (Inline Function Expansion):
https://msdn.microsoft.com/en-us/library/47238hez.aspx

[vstinner]

Ok, I confirm that Py_XINCREF() is properly inlined in Py_IncRef() with the latest version of my PR 10079.

I tested:

• gcc -O0
• clang -O0
• MSVC: x64 build in Debug mode

[serhiy-storchaka]

Is it guarantied that static inline functions will be inlined and will be not called as external functions from the Python library? The latter would break binary compatibility of extensions compiled with newer Python headers with older binary Python libraries.

[vstinner]

Is it guarantied that static inline functions will be inlined and will be not called as external functions from the Python library? The latter would break binary compatibility of extensions compiled with newer Python headers with older binary Python libraries.

First, I'm not sure that the "stable ABI" really works in practice, there are many issues:
https://pythoncapi.readthedocs.io/

Converting the macro to a static inline function is a minor step in the direction of a more stable API and ABI.

To come back to your question: depending on the compiler and compiler options, Py_INCREF/DECREF() can generate a *function call*. I tuned Py_STATIC_INLINE() to always inline Py_INCREF/DECREF() for GCC, clang and MSVC which are the major compilers used by Python on Windows, Linux, macOS and FreeBSD.

"static inline" is still something new to me. Maybe someone will come with a compiler with which it doesn't work as expected. IMHO we have to go through these issues, and it's only be testing for real that we will see these issues.

I mean, we *have to* get ride of these ugly macros used in Python header files. They are causing subtle bugs and are hard to maintain. See my first message for advantages of static inline functions.

[vstinner]

Is it guarantied that static inline functions will be inlined and will be not called as external functions from the Python library? The latter would break binary compatibility of extensions compiled with newer Python headers with older binary Python libraries.

Ok, I made more checks. In short, PR 10079 has no impact on the ABI compatibility.

I modified Py_STATIC_INLINE() to remove __attribute__((always_inline)).

In this case, the ./python binary contains multiple "instances" (what's the correct name for that?) of the "_Py_INCREF" inline function:
---
vstinner@apu$ readelf -sW ./python | c++filt -t |grep -E '(INC|DEC)REF'
42: 000000000041f908 49 FUNC LOCAL DEFAULT 13 _Py_INCREF
43: 000000000041f939 121 FUNC LOCAL DEFAULT 13 _Py_DECREF
109: 00000000004234cb 49 FUNC LOCAL DEFAULT 13 _Py_INCREF
...
5639: a486d 49 FUNC LOCAL DEFAULT 13 _Py_INCREF
5786: a7abc 49 FUNC LOCAL DEFAULT 13 _Py_INCREF
5801: a7f0f 49 FUNC LOCAL DEFAULT 13 _Py_INCREF
...
8126: 00000000006011c5 49 FUNC LOCAL DEFAULT 13 _Py_INCREF
8127: 00000000006011f6 121 FUNC LOCAL DEFAULT 13 _Py_DECREF
8140: 0000000000601971 49 FUNC LOCAL DEFAULT 13 _Py_INCREF
8141: 00000000006019a2 121 FUNC LOCAL DEFAULT 13 _Py_DECREF
...
---

These functions are *LOCAL*, I understand that they are not exported.

I also checked the _struct module:

vstinner@apu$ readelf -sW build/lib.linux-x86_64-3.8-pydebug/_struct.cpython-38dm-x86_64-linux-gnu.so | c++filt -t |grep -E '(INC|DEC)REF'
40: 0000000000002e99 55 FUNC LOCAL DEFAULT 11 _Py_INCREF
41: 0000000000002ed0 127 FUNC LOCAL DEFAULT 11 _Py_DECREF

Again, these functions are "LOCAL", not imported nor exported.

I undertand that _struct.so doesn't depend on libpython for INCREF/DECREF: it contains its own "implementation".

[vstinner]

Interesting article on inline/static inline and symbols:
https://gist.github.com/htfy96/50308afc11678d2e3766a36aa60d5f75

[vstinner]

New changeset 18618e6 by Victor Stinner in branch 'master':
bpo-35059: Add Py_STATIC_INLINE() macro (GH-10093)
18618e6