classification
Title: port extension modules' macros of `get_module_state()` to inline function.
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: corona10, petr.viktorin, rhettinger, shihai1991, tim.peters, vstinner
Priority: normal Keywords: patch

Created on 2020-03-15 13:42 by shihai1991, last changed 2020-03-16 18:24 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 19017 merged shihai1991, 2020-03-15 13:45
PR 19028 merged shihai1991, 2020-03-16 15:33
Messages (15)
msg364231 - (view) Author: hai shi (shihai1991) * (Python triager) Date: 2020-03-15 13:42
as victor and petr said in PR18613:inline function is more better than macros, so I plan move all extension modules' macros of `get_xx_state()` to inline function.

Note: some inline get_xx_state() can not be used directly in `tp_traverse`、`tp_free` and `tp_clear`(issue39824).
msg364278 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-03-16 03:08
Tim, do you approve of this kind of wholesale macro-to-inline function conversion?

My thought is that in most cases, there is no advantage to conversion and that sometimes there will be minor disadvantage in code generation when the macro is used across files.  I like inline functions as well as next person, but that doesn't mean all macros have to be rewritten.  Mostly, this seems like unnecessary code churn.
msg364315 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-16 13:15
New changeset f707d94af68a15afc27c1a9da5835f9456259fea by Hai Shi in branch 'master':
bpo-39968: Convert extension modules' macros of get_module_state() to inline functions (GH-19017)
https://github.com/python/cpython/commit/f707d94af68a15afc27c1a9da5835f9456259fea
msg364316 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-16 13:21
There are multiple good reasons to replace macros with static inline functions:

* Parameter types and return value are well defined
* Less error-prone: avoid completely https://gcc.gnu.org/onlinedocs/gcc-9.3.0/cpp/Macro-Pitfalls.html#Macro-Pitfalls
* Variables have a well defined scope, reduce the risk of unexpected side effects

I merged the PR 19017: it uses better names for the function and it adds assert(state != NULL).

Be careful of bpo-39824: in some cases, the module state *can* be NULL: but I checked, and it's not the case here. Moreover, bpo-39824 may prevent NULL module state (let's see ;-)).

Thanks Hai Shi!


> minor disadvantage in code generation

Micro-benchmarks and machine code analysis was done in previous issues replacing macros with static inline functions, and the outcome is that there is no effect on performance. See for example bpo-35059: performance critical Py_INCREF() and Py_DECREF() functions are now static inline functions.

If someone is curious about the the machine code, you should use -O3 with PGO+LTO, which is what should be used on Linux distributions. If static inline functions are not inlined for whatever reason, I suggest to tune the compiler rather than moving back to ugly macros.

If you use configure --enable-shared, I now also strongly suggest to use -fno-semantic-interposition: it's now used on Fedora and RHEL to build Python libpython.

Note: Support for "static inline" is now explicitly required by the PEP 7 to build Python, since Python 3.6. And it's more and more used in Python header files.
msg364317 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-16 13:23
Note: Hai Shi is working hard on converting modules to multiphase module initialization (PEP 489) which helps to cleanup the Python state at exit, and subinterpreters will likely benefit of that. This issue is related to this work.
msg364320 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-16 13:50
Oh, I forgot a cool advantage that I discovered late: debuggers and profilers understand inlined code and are able to get the name of the static inline function, whereas it's not possible to do that with macros. If I recall correctly, it works even if the function is inlined.
msg364321 - (view) Author: hai shi (shihai1991) * (Python triager) Date: 2020-03-16 14:00
Wow, thanks, victor. those msgs is very helpful to me.
msg364327 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-03-16 14:52
> cpython/Modules/readline.c:90:20: error: unknown type name
      'PyModule'
get_readline_state(PyModule *module)

Compile is failure after PR 19017 is merged on macOS.
msg364329 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-03-16 14:57
See also: https://github.com/python/cpython/runs/509226542#step:4:305
msg364332 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-03-16 15:19
I reopen this issue for the above problem
msg364333 - (view) Author: hai shi (shihai1991) * (Python triager) Date: 2020-03-16 15:34
> See also: https://github.com/python/cpython/runs/509226542#step:4:305
Oh, thanks, Dong-hee Na. I create a PR for this typo error.
msg364339 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-16 17:16
New changeset 5f104d56fa10f88098338b3f1ea74bcbe6924ca9 by Hai Shi in branch 'master':
bpo-39968: Fix a typo error in get_readline_state() (GH-19028)
https://github.com/python/cpython/commit/5f104d56fa10f88098338b3f1ea74bcbe6924ca9
msg364343 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-16 17:21
> Compile is failure after PR 19017 is merged on macOS.

Oh, macOS job was marked as success on PR 19017 :-(
https://github.com/python/cpython/runs/509226542

But I confirm that I can read there:

"""
Failed to build these modules:
_tkinter              readline         
"""
msg364347 - (view) Author: hai shi (shihai1991) * (Python triager) Date: 2020-03-16 17:53
> Oh, macOS job was marked as success on PR 19017 :-(

yeah, it's weird. This macOS job should be enhanced?
msg364349 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-16 18:24
> This macOS job should be enhanced?

setup.py skips optional dependencies like readline on purpose.

readline is built again onx86-64 macOS 3.x buildbot worker and test_readline passed. I close the issue.

Thanks for the quick fix.
History
Date User Action Args
2020-03-16 18:24:56vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg364349

stage: patch review -> resolved
2020-03-16 17:53:35shihai1991setmessages: + msg364347
2020-03-16 17:21:13vstinnersetmessages: + msg364343
2020-03-16 17:16:38vstinnersetmessages: + msg364339
2020-03-16 15:34:10shihai1991setmessages: + msg364333
2020-03-16 15:33:15shihai1991setstage: resolved -> patch review
pull_requests: + pull_request18377
2020-03-16 15:19:06corona10setstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg364332
2020-03-16 14:57:55corona10setmessages: + msg364329
2020-03-16 14:52:01corona10setnosy: + corona10
messages: + msg364327
2020-03-16 14:00:17shihai1991setmessages: + msg364321
2020-03-16 13:50:50vstinnersetmessages: + msg364320
2020-03-16 13:23:12vstinnersetmessages: + msg364317
2020-03-16 13:21:03vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg364316

stage: patch review -> resolved
2020-03-16 13:15:22vstinnersetmessages: + msg364315
2020-03-16 03:08:17rhettingersetnosy: + rhettinger, tim.peters
messages: + msg364278
2020-03-15 13:52:42shihai1991settitle: move extension modules' macros of `get_module_state()` to inline function. -> port extension modules' macros of `get_module_state()` to inline function.
2020-03-15 13:52:28shihai1991settitle: move extension modules' macros of `get_xx_state()` to inline function. -> move extension modules' macros of `get_module_state()` to inline function.
2020-03-15 13:45:05shihai1991setkeywords: + patch
stage: patch review
pull_requests: + pull_request18366
2020-03-15 13:42:06shihai1991create