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

Use METH_FASTCALL in str methods #73472

Closed
vstinner opened this issue Jan 16, 2017 · 20 comments
Closed

Use METH_FASTCALL in str methods #73472

vstinner opened this issue Jan 16, 2017 · 20 comments
Labels
3.7 performance

Comments

@vstinner
Copy link
Member

@vstinner vstinner commented Jan 16, 2017

BPO 29286
Nosy @vstinner, @larryhastings, @methane, @serhiy-storchaka, @1st1
PRs
  • #1886
  • Files
  • unicode_allow_kw.patch
  • ac_more_fastcalls.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 2017-02-01.16:45:08.895>
    created_at = <Date 2017-01-16.17:30:51.705>
    labels = ['3.7', 'performance']
    title = 'Use METH_FASTCALL in str methods'
    updated_at = <Date 2017-06-09.11:24:56.334>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2017-06-09.11:24:56.334>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-02-01.16:45:08.895>
    closer = 'vstinner'
    components = []
    creation = <Date 2017-01-16.17:30:51.705>
    creator = 'vstinner'
    dependencies = []
    files = ['46305', '46308']
    hgrepos = []
    issue_num = 29286
    keywords = ['patch']
    message_count = 20.0
    messages = ['285574', '285575', '285576', '285578', '285581', '285584', '285587', '285594', '285595', '285596', '285597', '285599', '285600', '285614', '286647', '286648', '286649', '286650', '286653', '295516']
    nosy_count = 6.0
    nosy_names = ['vstinner', 'larry', 'methane', 'python-dev', 'serhiy.storchaka', 'yselivanov']
    pr_nums = ['1886']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue29286'
    versions = ['Python 3.7']

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Jan 16, 2017

    Changes 27dc9a1c061e and 01b06ca45f64 converted the str (Unicode) methods to Argument Clinic, cool! But str methods taking more than one argument use positional-only arguments.

    Currently, Argument Clinic doesn't use METH_FASTCALL for these methods, but METH_VARARGS.

    There are two options:

    • Allow passing arguments as keywoards: str.replace(old='a', new='b')
    • Enhance Argument Clinic to use also METH_FASTCALL for functions using positional-only functions

    The goal is to speedup method calls. Example with str.replace():

    $ ./python-patch -m perf timeit '"a".replace("x", "y")' --duplicate=100 --compare-to ./python-ref
    python-ref: ..................... 132 ns +- 1 ns
    python-patch: ..................... 102 ns +- 2 ns

    Median +- std dev: [python-ref] 132 ns +- 1 ns -> [python-patch] 102 ns +- 2 ns: 1.30x faster (-23%)

    @vstinner vstinner added 3.7 performance labels Jan 16, 2017
    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Jan 16, 2017

    Background: see also the old issue bpo-17170 closed as rejected: "string method lookup is too slow".

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Jan 16, 2017

    The issue bpo-29263 "Implement LOAD_METHOD/CALL_METHOD for C functions" should also optimize C methods even more.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 16, 2017

    • Allow passing arguments as keywoards: str.replace(old='a', new='b')

    I think Guido explicitly stated that he doesn't like the idea to always allow keyword arguments for all methods. I.e. str.find('aaa') just reads better than str.find(needle='aaa'). Essentially, the idea is that for most of the builtins that accept one or two arguments, positional-only parameters are better.

    • Enhance Argument Clinic to use also METH_FASTCALL for functions using positional-only functions

    So this is the option to go with.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 16, 2017

    What is python-patch?

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Jan 16, 2017

    What is python-patch?

    It's Python patched with attached unicode_allow_kw.patch: allow to pass parameters as keywords for Unicode methods.

    @methane
    Copy link
    Member

    @methane methane commented Jan 16, 2017

    Parsing positional arguments for METH_KEYWORDS and METH_FASTCALL can be faster by
    http://bugs.python.org/issue29029

    @methane
    Copy link
    Member

    @methane methane commented Jan 17, 2017

    This patch makes AC produces more FASTCALL instead of VARARGS.

    When looking AC output, one downside is it produces many consts like:
    + static const char * const _keywords[] = {"", "", "", "", "", NULL};

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Jan 17, 2017

    ac_more_fastcalls.patch: Hum, I guess that your change on _PyStack_UnpackDict() is related to a bug in the function prototype. The function is unable to report failure if args is NULL. It changed the API in the change 38ab8572fde2.

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Jan 17, 2017

    New changeset d07fd6e6d449 by Victor Stinner in branch 'default':
    Rename _PyArg_ParseStack to _PyArg_ParseStackAndKeywords
    https://hg.python.org/cpython/rev/d07fd6e6d449

    New changeset 01c57ef1b651 by Victor Stinner in branch 'default':
    Add _PyArg_ParseStack() helper function
    https://hg.python.org/cpython/rev/01c57ef1b651

    New changeset 8bfec37ea86a by Victor Stinner in branch 'default':
    Add _PyArg_NoStackKeywords() helper function
    https://hg.python.org/cpython/rev/8bfec37ea86a

    New changeset 38ab8572fde2 by Victor Stinner in branch 'default':
    _PyStack_UnpackDict() now returns -1 on error
    https://hg.python.org/cpython/rev/38ab8572fde2

    New changeset de90f3d06bc9 by Victor Stinner in branch 'default':
    Argument Clinic: Use METH_FASTCALL for positionals
    https://hg.python.org/cpython/rev/de90f3d06bc9

    New changeset dea8922751a1 by Victor Stinner in branch 'default':
    Run Argument Clinic: METH_VARARGS=>METH_FASTCALL
    https://hg.python.org/cpython/rev/dea8922751a1

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Jan 17, 2017

    Naoki> This patch makes AC produces more FASTCALL instead of VARARGS.

    Oh, funny, I wrote the same patch :-) (almost)

    When looking AC output, one downside is it produces many consts like:

    • static const char * const _keywords[] = {"", "", "", "", "", NULL};

    Yeah, I got the same result. I tried to hack a _PyArg_ParseStack() function which doesn't take keyword argments (no kwnames), but I lost my mind in parser_init().

    So I decided to simply rebase my old patches from my random CPython fastcall forks to add a simple _PyArg_ParseStack(). My implementation doesn't use the super-fast _PyArg_Parser object, but at least it allows to use METH_FASTCALL. Compared to what we had previously (METH_VARARGS), it is an enhancement :-)

    I prefer to move step by step, since each commit is already big enough. It's also easier for me to maintain my forks if I push more changes upstream.

    So there is still room for improvement in _PyArg_ParseStack(), but I suggest to defer further optimizations to a new issue.

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Jan 17, 2017

    New changeset 3bf78c286daf by Victor Stinner in branch 'default':
    Add _PyArg_UnpackStack() function helper
    https://hg.python.org/cpython/rev/3bf78c286daf

    New changeset 905e902bd47e by Victor Stinner in branch 'default':
    Argument Clinic: Use METH_FASTCALL for boring positionals
    https://hg.python.org/cpython/rev/905e902bd47e

    New changeset 52acda52b353 by Victor Stinner in branch 'default':
    Run Argument Clinic: METH_VARARGS=>METH_FASTCALL
    https://hg.python.org/cpython/rev/52acda52b353

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Jan 17, 2017

    New changeset d07fd6e6d449 by Victor Stinner in branch 'default':
    Rename _PyArg_ParseStack to _PyArg_ParseStackAndKeywords
    https://hg.python.org/cpython/rev/d07fd6e6d449

    New changeset 01c57ef1b651 by Victor Stinner in branch 'default':
    Add _PyArg_ParseStack() helper function
    https://hg.python.org/cpython/rev/01c57ef1b651

    New changeset 8bfec37ea86a by Victor Stinner in branch 'default':
    Add _PyArg_NoStackKeywords() helper function
    https://hg.python.org/cpython/rev/8bfec37ea86a

    New changeset 38ab8572fde2 by Victor Stinner in branch 'default':
    _PyStack_UnpackDict() now returns -1 on error
    https://hg.python.org/cpython/rev/38ab8572fde2

    New changeset de90f3d06bc9 by Victor Stinner in branch 'default':
    Argument Clinic: Use METH_FASTCALL for positionals
    https://hg.python.org/cpython/rev/de90f3d06bc9

    New changeset dea8922751a1 by Victor Stinner in branch 'default':
    Run Argument Clinic: METH_VARARGS=>METH_FASTCALL
    https://hg.python.org/cpython/rev/dea8922751a1

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 17, 2017

    Oh, I have wrote almost the same patch before going to sleep yesteday! ;) But the building crashed (likely due to a bug in _PyStack_UnpackDict()) and it was too late to resolve this.

    I would prefer to rename "l" to "nargs" in PyArg_UnpackStack_impl and don't use upper case and namespace prefix in static functions.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Feb 1, 2017

    Recently, I saw complains that I push changes too fast without reviews. This issue is a good example. So let me elaborate myself a little bit on these changes.

    I'm not really proud of the "Rename _PyArg_ParseStack to _PyArg_ParseStackAndKeywords" change. It should be called _PyArg_ParseStackAndKeywords() from the beginning, but when I designed FASTCALL, my plan was to only check arguments (positional and keyword arguments) in the callee, and remove checks in the caller. I expected that all functions will get positional + keyword arguments.

    I didn't know that not accepting keywords but only positional arguments like in str.replace(), was a deliberate language design choice.

    So yes, for functions accepting only positional arguments, _PyArg_ParseStack() + _PyArg_NoStackKeywords() makes sense.

    IMHO the implementation of the new _PyArg_ParseStack() and _PyArg_NoStackKeywords() functions was simple enough to avoid a review, they reuse existing code with minimum changes.

    In general for FASTCALL, I only add private functions. I consider that it's ok to push code quickly, since the private property allows us to change these functions anytime.

    Again, I consider that the "Argument Clinic: Use METH_FASTCALL for positionals" change is simple enough to avoid a review, I don't think that they are many ways to use FASTCALL for functions using positional-only parameters. I mean, I only see a single way to implement it. Again, if someone sees a new way to handle such parameters, we can change it anytime, Argument Clinic and FASTCALL are stil private.

    More generally, FASTCALL requires a lot of tiny changes everywhere.

    GitHub will allow to get reviews on long patch series. It will be easier to handle large changes.

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Feb 1, 2017

    New changeset 758674087b12 by Victor Stinner in branch 'default':
    Issue bpo-29286: Rename private PyArg_UnpackStack_impl() to unpack_stack()
    https://hg.python.org/cpython/rev/758674087b12

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Feb 1, 2017

    Serhiy Storchaka: "Oh, I have wrote almost the same patch before going to sleep yesteday! ;) But the building crashed (likely due to a bug in _PyStack_UnpackDict()) and it was too late to resolve this."

    Oh, sorry that you wrote almost the same code. Well, at least it means that we agree on the design :-)

    Serhiy: "I would prefer to rename "l" to "nargs" in PyArg_UnpackStack_impl and don't use upper case and namespace prefix in static functions."

    Done.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Feb 1, 2017

    str type still has a few methods using METH_VARARGS (slower than METH_FASTCALL):

    • count()
    • find()
    • index()
    • rfind()
    • rindex()
    • startswith()
    • endswith()
    • format()

    Maybe I will propose a change later to convert these methods to Argument Clinic, but I consider that the initial issue "Currently, Argument Clinic doesn't use METH_FASTCALL for these methods, but METH_VARARGS" is now fixed, so I close the issue.

    Thanks Naoki and Serhiy.

    @vstinner vstinner closed this as completed Feb 1, 2017
    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Jun 9, 2017

    New changeset f0ff849 by Victor Stinner in branch '3.6':
    bpo-30524: Fix _PyStack_UnpackDict() (bpo-1886)
    f0ff849

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Apr 9, 2022

    New changeset 5910fd7 by Victor Stinner in branch 'master':
    Issue bpo-29286: Rename private PyArg_UnpackStack_impl() to unpack_stack()
    5910fd7

    @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
    3.7 performance
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants