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

Jump to bottom

port extension modules' macros of get_module_state() to inline function. #84149

Closed
shihai1991 opened this issue Mar 15, 2020 · 15 comments
Closed

port extension modules' macros of get_module_state() to inline function. #84149

shihai1991 opened this issue Mar 15, 2020 · 15 comments
Labels
3.9 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@shihai1991
Copy link
Member

BPO 39968
Nosy @tim-one, @rhettinger, @vstinner, @encukou, @corona10, @shihai1991
PRs
  • bpo-39968: move extension modules' macros of get_module_state() to inline function. #19017
  • bpo-39968: Fix a typo error of readline #19028
    		•

    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 2020-03-16.18:24:56.293>
created_at = <Date 2020-03-15.13:42:06.276>
labels = ['extension-modules', 'type-feature', '3.9']
title = "port extension modules' macros of `get_module_state()` to inline function."
updated_at = <Date 2020-03-16.18:24:56.292>
user = 'https://github.com/shihai1991'

    bugs.python.org fields:

    activity = <Date 2020-03-16.18:24:56.292>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-03-16.18:24:56.293>
closer = 'vstinner'
components = ['Extension Modules']
creation = <Date 2020-03-15.13:42:06.276>
creator = 'shihai1991'
dependencies = []
files = []
hgrepos = []
issue_num = 39968
keywords = ['patch']
message_count = 15.0
messages = ['364231', '364278', '364315', '364316', '364317', '364320', '364321', '364327', '364329', '364332', '364333', '364339', '364343', '364347', '364349']
nosy_count = 6.0
nosy_names = ['tim.peters', 'rhettinger', 'vstinner', 'petr.viktorin', 'corona10', 'shihai1991']
pr_nums = ['19017', '19028']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue39968'
versions = ['Python 3.9']
    @shihai1991
    Copy link
    Member Author

    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(bpo-39824).

    @shihai1991 shihai1991 added 3.9 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Mar 15, 2020
    @shihai1991 shihai1991 changed the title move extension modules' macros of get_xx_state() to inline function. move extension modules' macros of get_module_state() to inline function. Mar 15, 2020
    @shihai1991 shihai1991 changed the title move extension modules' macros of get_xx_state() to inline function. move extension modules' macros of get_module_state() to inline function. Mar 15, 2020
    @shihai1991 shihai1991 changed the title move extension modules' macros of get_module_state() to inline function. port extension modules' macros of get_module_state() to inline function. Mar 15, 2020
    @shihai1991 shihai1991 changed the title move extension modules' macros of get_module_state() to inline function. port extension modules' macros of get_module_state() to inline function. Mar 15, 2020
    @rhettinger
    Copy link
    Contributor

    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.

    @vstinner
    Copy link
    Member

    New changeset f707d94 by Hai Shi in branch 'master':
    bpo-39968: Convert extension modules' macros of get_module_state() to inline functions (GH-19017)
    f707d94

    @vstinner
    Copy link
    Member

    There are multiple good reasons to replace macros with static inline functions:

    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.

    @vstinner
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member

    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.

    @shihai1991
    Copy link
    Member Author

    Wow, thanks, victor. those msgs is very helpful to me.

    @corona10
    Copy link
    Member

    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.

    @corona10
    Copy link
    Member

    See also: https://github.com/python/cpython/runs/509226542#step:4:305

    @corona10
    Copy link
    Member

    I reopen this issue for the above problem

    @corona10 corona10 reopened this Mar 16, 2020
    @corona10 corona10 reopened this Mar 16, 2020
    @shihai1991
    Copy link
    Member Author

    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.

    @vstinner
    Copy link
    Member

    New changeset 5f104d5 by Hai Shi in branch 'master':
    bpo-39968: Fix a typo error in get_readline_state() (GH-19028)
    5f104d5

    @vstinner
    Copy link
    Member

    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
    """

    @shihai1991
    Copy link
    Member Author

    Oh, macOS job was marked as success on PR 19017 :-(

    yeah, it's weird. This macOS job should be enhanced?

    @vstinner
    Copy link
    Member

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.9 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @rhettinger @corona10 @shihai1991