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

Add METH_FASTCALL: new calling convention for C functions #71997

Closed
vstinner opened this issue Aug 20, 2016 · 18 comments
Closed

Add METH_FASTCALL: new calling convention for C functions #71997

vstinner opened this issue Aug 20, 2016 · 18 comments
Labels
type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

BPO 27810
Nosy @scoder, @vstinner, @serhiy-storchaka, @jdemeyer
Dependencies
  • bpo-27830: Add _PyObject_FastCallKeywords(): avoid the creation of a temporary dictionary for keyword arguments
  • Files
  • fastcall.patch
  • 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 2016-09-12.12:59:03.974>
    created_at = <Date 2016-08-20.00:32:46.209>
    labels = ['type-feature']
    title = 'Add METH_FASTCALL: new calling convention for C functions'
    updated_at = <Date 2019-05-14.20:40:00.365>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2019-05-14.20:40:00.365>
    actor = 'jdemeyer'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-09-12.12:59:03.974>
    closer = 'vstinner'
    components = []
    creation = <Date 2016-08-20.00:32:46.209>
    creator = 'vstinner'
    dependencies = ['27830']
    files = ['44517']
    hgrepos = []
    issue_num = 27810
    keywords = ['patch']
    message_count = 18.0
    messages = ['273173', '273336', '273339', '273340', '273341', '273409', '273679', '275447', '275516', '275555', '275560', '275826', '276033', '276046', '276047', '276048', '283332', '342518']
    nosy_count = 5.0
    nosy_names = ['scoder', 'vstinner', 'python-dev', 'serhiy.storchaka', 'jdemeyer']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27810'
    versions = ['Python 3.6']

    @vstinner
    Copy link
    Member Author

    The issue bpo-27128 added _PyObject_FastCall() to avoid the creation of temporary tuples when calling functions.

    I propose to add a new METH_FASTCALL calling convention. The example using METH_VARARGS | METH_KEYWORDS:
       PyObject* func(DirEntry *self, PyObject *args, PyObject *kwargs)
    becomes:
       PyObject* func(DirEntry *self, PyObject **args, int nargs, PyObject *kwargs)

    Using METH_VARARGS, args is a Python tuple. Using METH_FASTCALL, args is a C array of PyObject*, and there is a second nargs parameter.

    Later, Argument Clinic will be modified to *generate* code using the new METH_FASTCALL calling convention. Code written with Argument Clinic will only need to be updated by Argument Clinic to get the new faster calling convention (avoid the creation of a temporary tuple for positional arguments).

    This issue depends on the issue bpo-27809 "_PyObject_FastCall(): add support for keyword arguments". I will wait until this dependency is implemented, before working on the implementation of this part.

    For a full implementation, see my first attempt in the issue bpo-26814. I will extract the code from this branch to write a new patch.

    @vstinner vstinner added the type-feature A feature request or enhancement label Aug 20, 2016
    @scoder
    Copy link
    Contributor

    scoder commented Aug 22, 2016

    I agree that this would be cool. There is a tiny bit of a backwards compatibility concern as the new function signature would be incompatible with anything we had before, but I would guess that any code that chooses to bypass PyObject_Call() & friends would at least fall back to using it if it finds flags that it cannot handle. Cython code definitely does and always did, but there might be bugs. This change is the perfect way to "find" those. ;-)

    Anyway, Victor, I definitely appreciate your efforts in this direction.

    @vstinner
    Copy link
    Member Author

    There is a tiny bit of a backwards compatibility concern as the new function signature would be incompatible with anything we had before,

    Right. If you call directly PyCFunction functions, you will likely get quickly a crash. But... who call directly PyCFunction functions? Why not using the 30+ functions to call functions?

    Hopefully, it's easy to support METH_FASTCALL in an existing function getting a tuple:

       int nargs = (int)PyTuple_GET_SIZE(args);
       PyObject **stack = &PyTuple_GETITEM(args, 0);
       result = func(self, stack, nargs, kwargs);

    I guess that Cython calls directly PyCFunction. cpyext module of PyPy probably too. Ok, except of them, are you aware of other projects doing that?

    @scoder
    Copy link
    Contributor

    scoder commented Aug 22, 2016

    Extensive callback interfaces like map() come to mind, where a large number of calls becomes excessively time critical and might thus have made people implement their own special purpose calling code.

    However, I don't know any such code (outside of Cython) and I agree that this is such a special optimisation that there can't be many cases. If there are others, they'll likely be using the obvious fallback already, or otherwise just adapt when Py3.6 comes out and move on. The fix isn't complex at all.

    I do not consider this a road block.

    @vstinner
    Copy link
    Member Author

    Extensive callback interfaces like map() come to mind, where a large number of calls becomes excessively time critical and might thus have made people implement their own special purpose calling code.

    I didn't patch map_next() of Python/bltinmodule.c nor list.sort() for use fast call yet. With my full patch, map() and list.sort() are *faster*, but even more changes are needed ;-)

    https://bugs.python.org/issue26814#msg263999

    • map(lambda x: x, list(range(1000))): 18% faster
    • sorted(list, key=lambda x: x): 33% faster

    I'm now aware of map() or list.sort()/sorted() function rewritten from scratch for better performance, but I can imagine that someone did that. But do these specialized implementation use PyObject_Call() or "inline" PyCFunction_Call()?

    If someone "inlines" PyCFunction_Call(), be prepared to "backward incompatible changes", since it's common that CPython internals change in a subtle way.

    However, I don't know any such code (outside of Cython)

    I used GitHub to search for such code using "case METH_VARARGS". I found:

    In short, I found nothing. But I didn't search far, maybe there are a few other code bases using "case METH_VARARGS:"?

    @vstinner
    Copy link
    Member Author

    According to discussions in the issue bpo-27809, require a Python dict leads to inefficient code because a temporary dict may be required. The issue bpo-27830 proposes to pass keyword arguments as (key, value) pairs.

    It should be used for the new METH_FASTCALL calling convention.

    @vstinner
    Copy link
    Member Author

    I changed the dependency to the issue bpo-27830 "Add _PyObject_FastCallKeywords(): avoid the creation of a temporary dictionary for keyword arguments" which itself depends on the issue bpo-27213 "Rework CALL_FUNCTION* opcodes".

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 9, 2016

    New changeset a25c39873d93 by Victor Stinner in branch 'default':
    Issue bpo-27810: Add _PyCFunction_FastCallKeywords()
    https://hg.python.org/cpython/rev/a25c39873d93

    @vstinner
    Copy link
    Member Author

    fastcall.patch combines two changes:

    changeset: 103513:74abb8ddf7f2
    tag: tip
    user: Victor Stinner <victor.stinner@gmail.com>
    date: Fri Sep 09 17:40:38 2016 -0700
    files: Include/modsupport.h Python/getargs.c Tools/clinic/clinic.py
    description:
    Emit METH_FASTCALL code in Argument Clinic

    Issue bpo-27810:

    • Modify vgetargskeywordsfast() to work on a C array of PyObject* rather than
      working on a tuple directly.
    • Add _PyArg_ParseStack()
    • Argument Clinic now emits code using the new METH_FASTCALL calling convention

    changeset: 103512:d55abcddd194
    user: Victor Stinner <victor.stinner@gmail.com>
    date: Fri Sep 09 17:40:22 2016 -0700
    files: Include/abstract.h Include/methodobject.h Objects/abstract.c Objects/methodobject.c
    description:
    Add METH_FASTCALL calling convention

    Issue bpo-27810: Add a new calling convention for C functions:

        PyObject* func(PyObject *self, PyObject **args,
                       Py_ssize_t nargs, PyObject *kwnames);

    Where args is a C array of positional arguments followed by values of keyword
    arguments. nargs is the number of positional arguments, kwnames are keys of
    keyword arguments. kwnames can be NULL.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 10, 2016

    New changeset 86b0f5a900c7 by Victor Stinner in branch 'default':
    Add METH_FASTCALL calling convention
    https://hg.python.org/cpython/rev/86b0f5a900c7

    New changeset 633f850038c3 by Victor Stinner in branch 'default':
    Emit METH_FASTCALL code in Argument Clinic
    https://hg.python.org/cpython/rev/633f850038c3

    New changeset 97a68adbe826 by Victor Stinner in branch 'default':
    Issue bpo-27810: Rerun Argument Clinic on all modules
    https://hg.python.org/cpython/rev/97a68adbe826

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 10, 2016

    New changeset 3934e070c9db by Victor Stinner in branch 'default':
    Issue bpo-27810: Fix getargs.c compilation on Windows
    https://hg.python.org/cpython/rev/3934e070c9db

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 11, 2016

    New changeset 603bef7bb744 by Serhiy Storchaka in branch 'default':
    Issue bpo-27810: Regenerate Argument Clinic.
    https://hg.python.org/cpython/rev/603bef7bb744

    @vstinner
    Copy link
    Member Author

    Stefan Behnel: "There is a tiny bit of a backwards compatibility concern as the new function signature would be incompatible with anything we had before,"

    Python 3.6 will probably have two "fast call" calling convention:

    • _PyObject_FastCallDict(): expect a Python dict for keyword arguments
    • _PyObject_FastCallKeywods(): expect a Python tuple for keys of keyword arguments, keyword values are packed in the same array than positional arguments

    _PyObject_FastCallKeywods() is not really written to be called directly: Python/ceval.c calls you, but you may call _PyObject_FastCallKeywods() again "wrapper" functions, like functools.partial().

    Currently, tp_call (and tp_init and tp_new) still expects a (tuple, dict) for positional and keyword arguments, but later I will add something to also support METH_FASTCALL for callable objects. I just don't know yet what is the best option to make this change.

    --

    The main idea is implemented (implement METH_FASTCALL), I close the issue.

    I will open new issues for more specific changes, and maybe extend the API (especially tp_call).

    @serhiy-storchaka
    Copy link
    Member

    Could you please wrap "#define METH_FASTCALL 0x0080" with "#ifndef Py_LIMITED_API"/"#endif"? I think this method still is not stable, and we shouldn't encourage using it in third-party extensions.

    I would use two methods: just METH_FASTCALL and METH_FASTCALL|METH_KEYWORDS. The former is simpler and more stable, it takes just an array of positional parameters. The latter takes also keyword arguments, and the format of passing keyword arguments may be changed.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 12, 2016

    New changeset 08a500e8b482 by Victor Stinner in branch 'default':
    Issue bpo-27810: Exclude METH_FASTCALL from the stable API
    https://hg.python.org/cpython/rev/08a500e8b482

    @vstinner
    Copy link
    Member Author

    Could you please wrap "#define METH_FASTCALL 0x0080" with "#ifndef Py_LIMITED_API"/"#endif"?

    Sorry, this is a mistake. I tried to exclude all new symbols related to fast call from the stable API.

    It should now be fixed.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 15, 2016

    New changeset ecd218c41cd4 by Victor Stinner in branch 'default':
    Use _PyDict_NewPresized() in _PyStack_AsDict()
    https://hg.python.org/cpython/rev/ecd218c41cd4

    @jdemeyer
    Copy link
    Contributor

    Breakage due to the usage of borrowed references in _PyStack_UnpackDict(): bpo-36907

    @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
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants