bpo-36127: Argument Clinic: inline parsing code for keyword parameters. by serhiy-storchaka · Pull Request #12058

bpo-36127: Argument Clinic: inline parsing code for keyword parameters. by serhiy-storchaka · Pull Request #12058 · python/cpython

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it hard to use _PyTuple_ITEMS(args) instead? Maybe the generated .h file could #include "pycore_tupleobject.h"?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe always #include "pycore_tupleobject.h"?

Maybe make _PyTuple_ITEMS more accessible by default?

I'm trying to avoid that, I would prefer to reduce code using PyObject**.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too. ☹️

		...);
		PyAPI_FUNC(int) _PyArg_VaParseTupleAndKeywordsFast(PyObject , PyObject ,
		struct _PyArg_Parser *, va_list);
		PyAPI_FUNC(PyObject * const *) _PyArg_UnpackKeywords(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With removing just kwargs or kwnames, I do not think it is worth.