This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Converting collections.deque methods to Argument Clinic
Type: enhancement Stage: resolved
Components: Argument Clinic Versions: Python 3.10
process
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: Dennis Sweeney, corona10, larry, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2020-07-21 15:26 by corona10, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 21584 closed corona10, 2020-07-21 15:27
PR 24796 merged Dennis Sweeney, 2021-03-09 05:18
Messages (9)
msg374068 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-07-21 15:26
There was no performance regressin with this change.
msg374078 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-07-21 19:24
See also issue20180. 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.
msg374082 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-07-22 02:16
> collections.deque was intentionally not converted to Argument Clinic,

Oh, I see ..
msg374085 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-07-22 05:26
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.
msg374087 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-07-22 07:19
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.
msg388226 - (view) Author: Dennis Sweeney (Dennis Sweeney) * (Python committer) Date: 2021-03-07 06:26
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().
msg388323 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-03-09 02:12
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.
msg388806 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-03-16 04:02
New changeset 448801da96c70699e2ca0798efdfe86d45f37f06 by Dennis Sweeney in branch 'master':
bpo-41361: Optimized argument parsing for deque_rotate (GH-24796)
https://github.com/python/cpython/commit/448801da96c70699e2ca0798efdfe86d45f37f06
msg388807 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-03-16 04:02
Thanks for the PR :-)
History
Date User Action Args
2022-04-11 14:59:33adminsetgithub: 85533
2021-03-16 04:02:51rhettingersetmessages: + msg388807
2021-03-16 04:02:34rhettingersetmessages: + msg388806
2021-03-09 05:18:19Dennis Sweeneysetpull_requests: + pull_request23564
2021-03-09 02:12:09rhettingersetmessages: + msg388323
2021-03-07 06:26:43Dennis Sweeneysetnosy: + Dennis Sweeney
messages: + msg388226
2020-07-22 07:19:48rhettingersetstatus: open -> closed
resolution: not a bug
messages: + msg374087

stage: patch review -> resolved
2020-07-22 06:54:37rhettingersetassignee: rhettinger
2020-07-22 05:26:49serhiy.storchakasetmessages: + msg374085
2020-07-22 02:16:02corona10setmessages: + msg374082
2020-07-21 19:24:26serhiy.storchakasetnosy: + rhettinger, serhiy.storchaka
messages: + msg374078
2020-07-21 15:27:40corona10setkeywords: + patch
stage: patch review
pull_requests: + pull_request20725
2020-07-21 15:26:01corona10create