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 _PyObject_FastCallKeywords(): avoid the creation of a temporary dictionary for keyword arguments #72017

Closed
vstinner opened this issue Aug 22, 2016 · 11 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@vstinner
Copy link
Member

BPO 27830
Nosy @scoder, @vstinner, @vadmium, @serhiy-storchaka
Files
  • fastcall_keywords.patch
  • do_call.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-10.03:36:41.951>
    created_at = <Date 2016-08-22.22:44:24.153>
    labels = ['interpreter-core', 'performance']
    title = 'Add _PyObject_FastCallKeywords(): avoid the creation of a temporary dictionary for keyword arguments'
    updated_at = <Date 2017-01-18.10:28:05.257>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2017-01-18.10:28:05.257>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-09-10.03:36:41.951>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2016-08-22.22:44:24.153>
    creator = 'vstinner'
    dependencies = []
    files = ['44191', '44215']
    hgrepos = []
    issue_num = 27830
    keywords = ['patch']
    message_count = 11.0
    messages = ['273408', '273412', '273606', '273607', '273611', '273677', '273678', '273744', '274419', '275410', '285714']
    nosy_count = 5.0
    nosy_names = ['scoder', 'vstinner', 'python-dev', 'martin.panter', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue27830'
    versions = ['Python 3.6']

    @vstinner
    Copy link
    Member Author

    In the issue bpo-27809, I added support for keyword arguments to the function _PyObject_FastCallDict() (previously called _PyObject_FastCall). Keyword arguments are passed as a Python dictionary (dict).

    _PyObject_FastCallDict() is efficient when keyword arguments are not used. But when keyword arguments are used, the creation of a temporary dictionary to pass keyword arguments can defeat the "fastcall" optimization for positional arguments.

    See Serhiy Storchaka's comment:
    http://bugs.python.org/issue27809#msg273367

    Extract: "It may be more efficient to push values and names as continuous arrays (bpo-27213). PyArg_ParseTupleAndKeywords() accepts a tuple and a dict, but private function _PyArg_ParseTupleAndKeywordsFast() (bpo-27574) can be changed to accept positional and keyword arguments as continuous arrays: (int nargs, PyObject **args, int nkwargs, PyObject **kwnames, PyObject **kwargs)."

    And Stefan Behnel's comment:
    http://bugs.python.org/issue27809#msg273391

    Serhiy proposed to pass keyword arguments as (key, value) pairs in C array. We can probably use the simple "stack" format used in Python/ceval.c:

       PyObject* _PyObject_FastCallKeywords(PyObject *func, PyObject **stack, int nargs, int nkwargs)

    where nargs is number of positional arguments at the beginning of stack, and nkwargs is the number of (key, value) pairs at the end of stack.

    This function would be the fundation for a new C calling convention: METH_FASTCALL, see the issue bpo-27810.

    @vstinner vstinner added interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Aug 22, 2016
    @vstinner
    Copy link
    Member Author

    Here is a first patch:

    • Add _PyObject_FastCallKeywords(): main entry point
    • Add _PyCFunction_FastCallKeywords(): implementation for C function, but currently it's just an empty skeleton
    • Rename fast_function() to PyFunction_FastCallKeywords() and ajust its API

    In this first implementation, _PyObject_FastCallKeywords() has a full implementation for Python function, otherwise it only uses _PyObject_FastCallDict() fast path if the function has no keyword argument. In the last resort, it creates a temporary tuple and maybe also a temporary dict, to call PyObject_Call().

    To test the patch, I modified do_call() to call _PyObject_FastCallKeywords(). I'm not sure that it makes sense in term of performance at this stage.

    Serhiy: You can use this first implementation to experimental for fast argument parser for keyword arguments passed as (key, value) pairs ;-)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 24, 2016

    New changeset 5ec1095612d6 by Victor Stinner in branch 'default':
    Add _PyObject_FastCallKeywords()
    https://hg.python.org/cpython/rev/5ec1095612d6

    @vstinner
    Copy link
    Member Author

    I pushed a change adding _PyObject_FastCallKeywords(), but this function is not used yet.

    --

    I didn't push the do_call() change: I attach a new (shorter) patch which changes it.

    do_call.patch includes _PyCFunction_FastCallKeywords() for this line of Python/ceval.c:

    + C_TRACE(result, _PyCFunction_FastCallKeywords(func, stack, nargs, nkwargs));

    I don't understand exactly the purpose of C_TRACE(), so I chose to keep it to avoid any risk of regression.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 24, 2016

    New changeset 0ff25676505d by Victor Stinner in branch 'default':
    Issue bpo-27830: Fix _PyObject_FastCallKeywords()
    https://hg.python.org/cpython/rev/0ff25676505d

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 25, 2016

    New changeset ffcfa4f005a3 by Victor Stinner in branch 'default':
    Issue bpo-27830: Revert, remove _PyFunction_FastCallKeywords()
    https://hg.python.org/cpython/rev/ffcfa4f005a3

    @vstinner
    Copy link
    Member Author

    New changeset ffcfa4f005a3 by Victor Stinner in branch 'default':
    Issue bpo-27830: Revert, remove _PyFunction_FastCallKeywords()
    https://hg.python.org/cpython/rev/ffcfa4f005a3

    I'm sorry, I misunderstood Serhiy's comment. I understood that Serhiy proposed to reuse the current format "PyObject **stack, int nargs, int nkwargs", but in fact he proposes something new in the issue bpo-27213.

    IMO the issue bpo-27213 can be a major enhancement for Python 3.6. I prefer to wait until the issue bpo-27213 is implemented (that we agreed on the new format), before working again on _PyObject_FastCallKeywords().

    @vstinner vstinner changed the title Add _PyObject_FastCallKeywords(): pass keyword arguments as (key, value) pairs Add _PyObject_FastCallKeywords(): avoid the creation of a temporary dictionary for keyword arguments Aug 25, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Aug 27, 2016

    FYI revision ffcfa4f005a3 removed the code that uses your new _PyStack_AsDict() function, so now there is a compiler warning:

    Objects/abstract.c:2313:1: warning: ‘_PyStack_AsDict’ defined but not used [-Wunused-function]
    _PyStack_AsDict(PyObject **stack, Py_ssize_t nkwargs, PyObject *func)
    ^~~~~~~~~~~~~~~

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 5, 2016

    New changeset 119af59911a0 by Victor Stinner in branch 'default':
    Issue bpo-27830: Remove unused _PyStack_AsDict()
    https://hg.python.org/cpython/rev/119af59911a0

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 9, 2016

    New changeset 18d2d128cd1e by Victor Stinner in branch 'default':
    Add _PyObject_FastCallKeywords()
    https://hg.python.org/cpython/rev/18d2d128cd1e

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 18, 2017

    New changeset 1b7a5bdd05ed by Victor Stinner in branch 'default':
    _PyObject_FastCallKeywords() now checks the result
    https://hg.python.org/cpython/rev/1b7a5bdd05ed

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants