Converting collections.deque methods to Argument Clinic #85533

Activity

1. corona10 commented on Jul 21, 2020

   

   corona10

   on Jul 21, 2020

   MemberAuthor

   More actions

   There was no performance regressin with this change.
2. corona10

   added

   3.10 (EOL)end of lifeend of life

   topic-argument-clinic

   type-featureA feature request or enhancementA feature request or enhancement

   on Jul 21, 2020
3. serhiy-storchaka commented on Jul 21, 2020

   

   serhiy-storchaka

   on Jul 21, 2020

   Member

   More actions

   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.
4. corona10 commented on Jul 22, 2020

   

   corona10

   on Jul 22, 2020

   MemberAuthor

   More actions

   collections.deque was intentionally not converted to Argument Clinic,

   Oh, I see ..
5. serhiy-storchaka commented on Jul 22, 2020

   

   serhiy-storchaka

   on Jul 22, 2020

   Member

   More actions

   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.
6. rhettinger

   assigned

   rhettinger

   on Jul 22, 2020
7. rhettinger commented on Jul 22, 2020

   

   rhettinger

   on Jul 22, 2020

   Contributor

   More actions

   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.
8. rhettinger

   closed this as completedon Jul 22, 2020
9. rhettinger

   added

   invalid

   on Jul 22, 2020
10. rhettinger

    closed this as completedon Jul 22, 2020
11. rhettinger

    added

    invalid

    on Jul 22, 2020
12. sweeneyde commented on Mar 7, 2021

    

    sweeneyde

    on Mar 7, 2021

    Member

    More actions

    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().
13. rhettinger commented on Mar 9, 2021

    

    rhettinger

    on Mar 9, 2021

    Contributor

    More actions

    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.
14. rhettinger commented on Mar 16, 2021

    

    rhettinger

    on Mar 16, 2021

    Contributor

    More actions

    New changeset 448801d by Dennis Sweeney in branch 'master':
    bpo-41361: Optimized argument parsing for deque_rotate (GH-24796)
    448801d
15. rhettinger commented on Mar 16, 2021

    

    rhettinger

    on Mar 16, 2021

    Contributor

    More actions

    Thanks for the PR :-)
16. ezio-melotti

    transferred this issue fromon Apr 10, 2022