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

Converting collections.deque methods to Argument Clinic #85533

Closed
corona10 opened this issue Jul 21, 2020 · 9 comments
Closed

Converting collections.deque methods to Argument Clinic #85533

corona10 opened this issue Jul 21, 2020 · 9 comments
Assignees
Labels
3.10 only security fixes topic-argument-clinic type-feature A feature request or enhancement

Comments

@corona10
Copy link
Member

BPO 41361
Nosy @rhettinger, @larryhastings, @serhiy-storchaka, @corona10, @sweeneyde
PRs
  • bpo-41361: Converting collections.deque methods to Argument Clinic #21584
  • bpo-41361: Optimized argument parsing for deque_rotate #24796
  • 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 = 'https://github.com/rhettinger'
    closed_at = <Date 2020-07-22.07:19:48.773>
    created_at = <Date 2020-07-21.15:26:01.614>
    labels = ['type-feature', 'invalid', 'expert-argument-clinic', '3.10']
    title = 'Converting collections.deque methods to Argument Clinic'
    updated_at = <Date 2021-03-16.04:02:51.139>
    user = 'https://github.com/corona10'

    bugs.python.org fields:

    activity = <Date 2021-03-16.04:02:51.139>
    actor = 'rhettinger'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2020-07-22.07:19:48.773>
    closer = 'rhettinger'
    components = ['Argument Clinic']
    creation = <Date 2020-07-21.15:26:01.614>
    creator = 'corona10'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41361
    keywords = ['patch']
    message_count = 9.0
    messages = ['374068', '374078', '374082', '374085', '374087', '388226', '388323', '388806', '388807']
    nosy_count = 5.0
    nosy_names = ['rhettinger', 'larry', 'serhiy.storchaka', 'corona10', 'Dennis Sweeney']
    pr_nums = ['21584', '24796']
    priority = 'normal'
    resolution = 'not a bug'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue41361'
    versions = ['Python 3.10']

    @corona10
    Copy link
    Member Author

    There was no performance regressin with this change.

    @corona10 corona10 added 3.10 only security fixes topic-argument-clinic type-feature A feature request or enhancement labels Jul 21, 2020
    @serhiy-storchaka
    Copy link
    Member

    See also bpo-20180. collections.deque was intentionally not converted to Argument Clinic, but some of methods were made using more efficient code for parsing arguments by inlining the code generated by Argument Clinic at that time.

    @corona10
    Copy link
    Member Author

    collections.deque was intentionally not converted to Argument Clinic,

    Oh, I see ..

    @serhiy-storchaka
    Copy link
    Member

    Now Argument Clinic generates more efficient (but more cumbersome) code, so there may be new reasons of using it. Please test the performance of the deque constructor (deque(), deque(()), deque((), 10), deque((), maxlen=10)), methods index() and rotate(), builtins iter() and reversed(). It is hard to test insert() because it mutates the deque and I do not expect anything beside noise for other methods.

    @rhettinger
    Copy link
    Contributor

    I think we should skip applying the clinic to the code. It jumbles the code quite a bit but doesn't give any real benefit.

    @sweeneyde
    Copy link
    Member

    If the argument clinic is too disruptive, would it be okay to inline the equivalent code like this?

    diff --git a/Modules/_collectionsmodule.c b/Modules/_collectionsmodule.c
    index 90bafb0ea8..d75388abc8 100644
    --- a/Modules/_collectionsmodule.c
    +++ b/Modules/_collectionsmodule.c
    @@ -880,9 +880,19 @@ deque_rotate(dequeobject *deque, PyObject *const *args, Py_ssize_t nargs)
     {
         Py_ssize_t n=1;
    -    if (!_PyArg_ParseStack(args, nargs, "|n:rotate", &n)) {
    +    if (!_PyArg_CheckPositional("deque.rotate", nargs, 0, 1)) {
             return NULL;
         }
    +    if (nargs) {
    +        PyObject *index = _PyNumber_Index(args[0]);
    +        if (index == NULL) {
    +            return NULL;
    +        }
    +        n = PyLong_AsSsize_t(index);
    +        if (n == -1 && PyErr_Occurred()) {
    +            return NULL;
    +        }
    +    }
         if (!_deque_rotate(deque, n))
             Py_RETURN_NONE;

    Benchmarks for this change:

    .\python.bat -m pyperf timeit -s "from collections import deque; d = deque(range(100))" "d.rotate()"
    Before: Mean +- std dev: 51.2 ns +- 0.9 ns
    After:  Mean +- std dev: 39.3 ns +- 0.3 ns
    
    .\python.bat -m pyperf timeit -s "from collections import deque; d = deque(range(100))" "d.rotate(-1)"
    Before: Mean +- std dev: 64.5 ns +- 0.3 ns
    After:  Mean +- std dev: 46.2 ns +- 0.3 ns
    
    .\python.bat -m pyperf timeit -s "from collections import deque; d = deque(range(100))" "d.rotate(1)"
    Before: Mean +- std dev: 64.4 ns +- 0.3 ns
    After:  Mean +- std dev: 45.7 ns +- 0.2 ns
    

    Similar changes could apply to deque.insert() and deque.index().

    @rhettinger
    Copy link
    Contributor

    Go ahead with in-lining the argument handling for rotate(). The no argument form and the +1 and the -1 case are important enough to warrant the change.

    Please skip index() and insert() which aren't essential methods. Also, those methods aren't called with end-point specific arguments, so the argument processing time doesn't dominate and it isn't worth it.

    @rhettinger
    Copy link
    Contributor

    New changeset 448801d by Dennis Sweeney in branch 'master':
    bpo-41361: Optimized argument parsing for deque_rotate (GH-24796)
    448801d

    @rhettinger
    Copy link
    Contributor

    Thanks for the PR :-)

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes topic-argument-clinic type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants