Skip to content

bpo-36127: Argument Clinic: inline parsing code for keyword parameters. #12058

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 14, 2019
Merged

bpo-36127: Argument Clinic: inline parsing code for keyword parameters. #12058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
06f7117
bpo-36127: Argument Clinic: inline parsing code for keyword parameters.
serhiy-storchaka Feb 18, 2019
8f8b05a
Add tests and the What's New entry.
serhiy-storchaka Feb 27, 2019
41e0a00
Merge branch 'master' into clinic-inline-unpack-keywords
serhiy-storchaka Mar 7, 2019
0e83072
Merge branch 'master' into clinic-inline-unpack-keywords
serhiy-storchaka Mar 8, 2019
acf77d9
Fix uses of %d.
serhiy-storchaka Mar 10, 2019
ae9283a
Merge branch 'master' into clinic-inline-unpack-keywords
serhiy-storchaka Mar 10, 2019
2521f95
Merge branch 'master' into clinic-inline-unpack-keywords
serhiy-storchaka Mar 13, 2019
0af20f3
Refactor _PyArg_UnpackKeywords().
serhiy-storchaka Mar 13, 2019
b7c9f60
Break the loop in _PyArg_UnpackKeywords() early.
serhiy-storchaka Mar 14, 2019
0ec3da8
Merge branch 'master' into clinic-inline-unpack-keywords
serhiy-storchaka Mar 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Doc/whatsnew/3.8.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,11 @@ Optimizations
(Contributed by Stefan Behnel, Pablo Galindo Salgado, Raymond Hettinger,
Neil Schemenauer, and Serhiy Storchaka in :issue:`36012`.)

* Reduced an overhead of converting arguments passed to many builtin functions
and methods. This sped up calling some simple builtin functions and
methods up to 20--50%. (Contributed by Serhiy Storchaka in :issue:`23867`,
:issue:`35582` and :issue:`36127`.)


Build and C API Changes
=======================
Expand Down
12 changes: 12 additions & 0 deletions Include/modsupport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,18 @@ PyAPI_FUNC(int) _PyArg_ParseStackAndKeywords(
...);
PyAPI_FUNC(int) _PyArg_VaParseTupleAndKeywordsFast(PyObject *, PyObject *,
struct _PyArg_Parser *, va_list);
PyAPI_FUNC(PyObject * const *) _PyArg_UnpackKeywords(
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

PyObject *const *args, Py_ssize_t nargs,
PyObject *kwargs, PyObject *kwnames,
struct _PyArg_Parser *parser,
int minpos, int maxpos, int minkw,
PyObject **buf);
#define _PyArg_UnpackKeywords(args, nargs, kwargs, kwnames, parser, minpos, maxpos, minkw, buf) \
Copy link
Member

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

Copy link
Member Author

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.

(((minkw) == 0 && (kwargs) == NULL && (kwnames) == NULL && \
(minpos) <= (nargs) && (nargs) <= (maxpos) && args != NULL) ? (args) : \
_PyArg_UnpackKeywords((args), (nargs), (kwargs), (kwnames), (parser), \
(minpos), (maxpos), (minkw), (buf)))

void _PyArg_Fini(void);
#endif /* Py_LIMITED_API */

Expand Down
Loading