bpo-36127: Argument Clinic: inline parsing code for keyword parameters.#12058
bpo-36127: Argument Clinic: inline parsing code for keyword parameters.#12058serhiy-storchaka merged 10 commits intopython:masterpython/cpython:masterfrom serhiy-storchaka:clinic-inline-unpack-keywordsserhiy-storchaka/cpython:clinic-inline-unpack-keywordsCopy head branch name to clipboard
Conversation
vstinner
left a comment
There was a problem hiding this comment.
I like the speedup :-) A first round of review.
| struct _PyArg_Parser *parser, | ||
| int minpos, int maxpos, int minkw, | ||
| PyObject **buf); | ||
| #define _PyArg_UnpackKeywords(args, nargs, kwargs, kwnames, parser, minpos, maxpos, minkw, buf) \ |
There was a problem hiding this comment.
I dislike such long and complex macros. Can't you use "static inline" function instead? You should rename _PyArg_UnpackKeywords function (ex: rename it to _PyArg_UnpackKeywords_impl).
There was a problem hiding this comment.
The "static inline" function would be even longer (you need to specify types of all parameters. The macro guaranties inlining the code. And I dislike non-local names like _PyArg_UnpackKeywords_impl.
| if has_optional_kw: | ||
| declarations += "\nPy_ssize_t noptargs = nargs + (kwargs ? PyDict_GET_SIZE(kwargs) : 0) - %d;" % (min_pos + min_kw_only) | ||
| parser_code = [normalize_snippet(""" | ||
| fastargs = _PyArg_UnpackKeywords(_PyTuple_CAST(args)->ob_item, nargs, kwargs, NULL, &_parser, %d, %d, %d, argsbuf); |
There was a problem hiding this comment.
Would it hard to use _PyTuple_ITEMS(args) instead? Maybe the generated .h file could #include "pycore_tupleobject.h"?
There was a problem hiding this comment.
Iwould like to use _PyTuple_ITEMS, but it is not available in all files, and I do not know how to make Argument Clinic adding #include "pycore_tupleobject.h". Currently it does not add any includes. And I do not want to add this include manually in all these files.
Maybe make _PyTuple_ITEMS more accessible by default?
There was a problem hiding this comment.
Maybe always #include "pycore_tupleobject.h"?
Maybe make
_PyTuple_ITEMSmore accessible by default?
I'm trying to avoid that, I would prefer to reduce code using PyObject**.
There was a problem hiding this comment.
Do you know how to add #include "pycore_tupleobject.h"? The code of Argument Clinic is too complex and I do not want to break it.
There was a problem hiding this comment.
I tried... but failed to add #include "pycore_tupleobject.h". I never really looked at Argument Clinic. Maybe just ignore my comment and access directly PyTupleObject.ob_item. It can be fixed later if needed ;-)
| ...); | ||
| PyAPI_FUNC(int) _PyArg_VaParseTupleAndKeywordsFast(PyObject *, PyObject *, | ||
| struct _PyArg_Parser *, va_list); | ||
| PyAPI_FUNC(PyObject * const *) _PyArg_UnpackKeywords( |
There was a problem hiding this comment.
Would it make sense to have one function for dict kwargs and one function for tuple kwnames?
Maybe the implementation can remain unchanged, but at least in term of API it would look better. In the generated code, we never have kwargs and kwnames defined at the same time. It's either one or the other.
Maybe it doesn't make sense, I'm not sure.
There was a problem hiding this comment.
I considered this idea from the beginning.
But currently PyTuple_GET_SIZE(args) is evaluated only once for __new__ and __init__. It is passes to _PyArg_UnpackKeywords and used in the following code. With specialized macros/functions it will be evaluated several times. I do not know how this will affect performance, but this looks like a waste of CPU cycles.
There was a problem hiding this comment.
I don't understand why you are talking about PyTuple_GET_SIZE(args). I'm not proposing to remove nargs parameter from _PyArg_UnpackKeywords().
I'm asking if it would make sense to have _PyArg_UnpackKwdict() for dict kwargs, and _PyArg_UnpackKwnames() for tuple kwnames. But maybe it's not worth it.
There was a problem hiding this comment.
With removing just kwargs or kwnames, I do not think it is worth.
https://bugs.python.org/issue36127