Repository navigation
Converting collections.deque methods to Argument Clinic #85533
Description
Activity
There was no performance regressin with this change.
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.
collections.deque was intentionally not converted to Argument Clinic,
Oh, I see ..
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.
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.
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 nsSimilar changes could apply to deque.insert() and deque.index().
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.
Thanks for the PR :-)
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:
bugs.python.org fields: