gh-101123: Add signature for the math.hypot #101124
Conversation
Modules/mathmodule.c
Outdated
| if (nargs > NUM_STACK_ELEMS) { | ||
| coordinates = (double *) PyObject_Malloc(nargs * sizeof(double)); | ||
| if (coordinates == NULL) { | ||
| return PyErr_NoMemory(); | ||
| } | ||
| } | ||
| for (i = 0; i < nargs; i++) { | ||
| item = args[i]; | ||
| item = PyTuple_GetItem(args, i); |
There was a problem hiding this comment.
This might affect performance. Can you please measure it?
There was a problem hiding this comment.
Indeed, there is a difference.
$ ./python -m timeit -s 'from math import hypot as h' -s 'a = list(range(1, 10))' 'h(*a)'
500000 loops, best of 5: 650 nsec per loop
$ ./python -m timeit -s 'from math import hypot as h' -s 'a = list(range(1, 100))' 'h(*a)'
50000 loops, best of 5: 4.54 usec per loop
$ ./python -m timeit -s 'from math import hypot as h' -s 'a = list(range(1, 1000))' 'h(*a)'
5000 loops, best of 5: 45.1 usec per loop
$ git checkout fix-101123
Switched to branch 'fix-101123'
Your branch is up to date with 'github/fix-101123'.
$ make -s
Checked 111 modules (30 built-in, 80 shared, 1 n/a on linux-x86_64, 0 disabled, 0 missing, 0 failed on import)
$ ./python -m timeit -s 'from math import hypot as h' -s 'a = list(range(1, 10))' 'h(*a)'
500000 loops, best of 5: 950 nsec per loop
$ ./python -m timeit -s 'from math import hypot as h' -s 'a = list(range(1, 100))' 'h(*a)'
50000 loops, best of 5: 6.43 usec per loop
$ ./python -m timeit -s 'from math import hypot as h' -s 'a = list(range(1, 1000))' 'h(*a)'
5000 loops, best of 5: 65.6 usec per loop
So, probably we should use the patch from the issue. On another hand, the comment ("AC: cannot convert yet, waiting for *args support") - seems to be misleading and must be removed.
There was a problem hiding this comment.
You can use string-based docs to populate __text_signature__. It will fix your problem with inspect.signature with 0 runtime cost. Example from dict.get:
PyDoc_STRVAR(dict_get__doc__,
"get($self, key, default=None, /)\n"
"--\n"
"\n"
"Return the value for key if key is in the dictionary, else default.");There was a problem hiding this comment.
You can use string-based docs to populate text_signature.
Yes, I understand. That seems to be the solution, presented in the issue description, isn't?
The question was: should we keep the mentioned comment for math_hypot? It seems to be misleading for me (issue reference would be more helpful here, if it is an open problem), as there is no principal problems to convert this function to the Argument Clinic.
There was a problem hiding this comment.
Yes, I think it should be changed to: Not converted to AC because it affects performance
There was a problem hiding this comment.
Could @rhettinger bless this change? The comment is coming from #8474 and I'm not sure he meant performance issues with AC (rather something like #64490).
PS: AC stuff reverted.
Timings with PyTuple_GET_ITEM:
$ ./python -m timeit -s 'from math import hypot as h' -s 'a = list(range(1, 10))' 'h(*a)'
500000 loops, best of 5: 814 nsec per loop
$ ./python -m timeit -s 'from math import hypot as h' -s 'a = list(range(1, 100))' 'h(*a)'
50000 loops, best of 5: 5.16 usec per loop
$ ./python -m timeit -s 'from math import hypot as h' -s 'a = list(range(1, 1000))' 'h(*a)'
5000 loops, best of 5: 53.2 usec per loop
Note: this does conversion of the math.hypot to the Argument Clinic. Alternative solution mentioned in the issue description.