Converting collections.deque methods to Argument Clinic

[corona10]

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]

There was no performance regressin with this change.

[serhiy-storchaka]

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]

collections.deque was intentionally not converted to Argument Clinic,

Oh, I see ..

[serhiy-storchaka]

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]

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]

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]

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]

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

[rhettinger]

Thanks for the PR :-)