Skip to content

gh-90370: Avoid temporary varargs tuple creation in argument passing #30312

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

Closed
colorfulappl wants to merge 27 commits into from
Closed

gh-90370: Avoid temporary varargs tuple creation in argument passing #30312

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
b68176d
Avoid temporary `varargs` tuple creation in argument passing
colorfulappl Dec 31, 2021
43b9410
Optimize argument passing of builtin print() with modified Argument C…
colorfulappl Dec 31, 2021
9b37a77
Revert modification of PyAPI_FUNC _PyArg_UnpackKeywordsWithVararg to …
colorfulappl Jan 4, 2022
46e4472
📜🤖 Added by blurb_it.
blurb-it[bot] Jan 4, 2022
e06f343
Revert _PyArg_UnpackKeywordsWithVararg and add _PyArg_UnpackKeywordsW…
colorfulappl Jan 4, 2022
d35410b
Fix a bug which allows more than one varargs
colorfulappl Feb 22, 2022
0a9fe91
Do not copy posargs and vararg during argument parsing
colorfulappl Mar 22, 2022
9188052
Rename _PyArg_UnpackKeywordsWithVarargFast, it returns kwargs only now
colorfulappl Mar 23, 2022
d245ec0
Check type of varargs when generating argument parser
colorfulappl Mar 23, 2022
8d16685
Fix varargssize calculation in class init and add test cases
colorfulappl Mar 23, 2022
f4213ea
Merge branch 'main' into opt_ac
colorfulappl Mar 23, 2022
156f8d6
Rerun make clinic
colorfulappl Mar 23, 2022
c32aed4
Edit documentation
colorfulappl Mar 23, 2022
7ef5623
Merge branch 'main' into opt_ac
colorfulappl Feb 3, 2023
3ac2821
Fix errors introduced by merging and rerun make clinic
colorfulappl Feb 3, 2023
f63a704
Update news
colorfulappl Feb 3, 2023
8f61f5e
Fix varargssize in new_or_init
colorfulappl Feb 6, 2023
1eff215
Simplify the code a bit
colorfulappl Feb 6, 2023
5baf0f5
Optimize generated varargssize assignment
colorfulappl Feb 6, 2023
30c0606
Fix argument passing in new_or_init
colorfulappl Feb 7, 2023
d9e7408
Add tests for class method `__new__`
colorfulappl Feb 7, 2023
0f04a95
Merge branch 'main' into opt_ac
colorfulappl Feb 7, 2023
85a8ac5
Correct test function name
colorfulappl Feb 8, 2023
a9d1424
Fix vararg parsing when its name is not `args`
colorfulappl Feb 8, 2023
d6ff01a
Merge branch 'main' into opt_ac
colorfulappl Feb 8, 2023
597152e
Add testcases for undeclared keyword arguments
colorfulappl Feb 8, 2023
3119fa6
Merge branch 'main' into opt_ac
erlend-aasland Apr 29, 2023
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
Prev Previous commit
Next Next commit
Do not copy posargs and vararg during argument parsing
  • Loading branch information
@colorfulappl
colorfulappl committed Mar 22, 2022
commit 0a9fe9189238128810936e47257560a96753de85
51 changes: 26 additions & 25 deletions Lib/test/clinic.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3331,17 +3331,15 @@ static PyObject *
test_vararg_and_posonly(PyObject *module, PyObject *const *args, Py_ssize_t nargs)
{
PyObject *return_value = NULL;
Py_ssize_t varargssize = Py_MAX(nargs - 1, 0);
PyObject *a;
PyObject *const *__clinic_args = NULL;
PyObject *const *__clinic_args;

if (!_PyArg_CheckPositional("test_vararg_and_posonly", nargs, 1, PY_SSIZE_T_MAX)) {
goto exit;
}
a = args[0];
__clinic_args = PyTuple_New(nargs - 1);
for (Py_ssize_t i = 0; i < nargs - 1; ++i) {
PyTuple_SET_ITEM(__clinic_args, i, args[1 + i]);
}
__clinic_args = args + 1;
return_value = test_vararg_and_posonly_impl(module, a, varargssize, __clinic_args);

exit:
Expand All @@ -3351,7 +3349,7 @@ exit:
static PyObject *
test_vararg_and_posonly_impl(PyObject *module, PyObject *a,
Py_ssize_t varargssize, PyObject *const *args)
/*[clinic end generated code: output=6a6c69c6ff7deb9b input=08dc2bf7afbf1613]*/
/*[clinic end generated code: output=23a9d5c5c3cbc015 input=08dc2bf7afbf1613]*/

/*[clinic input]
test_vararg
Expand Down Expand Up @@ -3382,16 +3380,17 @@ test_vararg(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject
static _PyArg_Parser _parser = {NULL, _keywords, "test_vararg", 0};
PyObject *argsbuf[2];
Py_ssize_t varargssize = Py_MAX(nargs - 1, 0);
PyObject *const *fastargs;
Py_ssize_t noptargs = 0 + (kwnames ? PyTuple_GET_SIZE(kwnames) : 0) - 1;
PyObject *a;
PyObject *const *__clinic_args = NULL;
PyObject *const *__clinic_args;

args = _PyArg_UnpackKeywordsWithVarargFast(args, nargs, NULL, kwnames, &_parser, 1, 1, 0, 1, varargssize, argsbuf);
if (!args) {
fastargs = _PyArg_UnpackKeywordsWithVarargFast(args, nargs, NULL, kwnames, &_parser, 1, 1, 0, 1, varargssize, argsbuf);
if (!fastargs) {
goto exit;
}
a = args[0];
__clinic_args = (PyObject *const *)args[1];
__clinic_args = args + 1;
return_value = test_vararg_impl(module, a, varargssize, __clinic_args);

exit:
Expand All @@ -3401,7 +3400,7 @@ exit:
static PyObject *
test_vararg_impl(PyObject *module, PyObject *a, Py_ssize_t varargssize,
PyObject *const *args)
/*[clinic end generated code: output=b9cb9484bd7303db input=81d33815ad1bae6e]*/
/*[clinic end generated code: output=67d632b0b77d2c2b input=81d33815ad1bae6e]*/

/*[clinic input]
test_vararg_with_default
Expand Down Expand Up @@ -3434,21 +3433,22 @@ test_vararg_with_default(PyObject *module, PyObject *const *args, Py_ssize_t nar
static _PyArg_Parser _parser = {NULL, _keywords, "test_vararg_with_default", 0};
PyObject *argsbuf[3];
Py_ssize_t varargssize = Py_MAX(nargs - 1, 0);
PyObject *const *fastargs;
Py_ssize_t noptargs = 0 + (kwnames ? PyTuple_GET_SIZE(kwnames) : 0) - 1;
PyObject *a;
PyObject *const *__clinic_args = NULL;
PyObject *const *__clinic_args;
int b = 0;

args = _PyArg_UnpackKeywordsWithVarargFast(args, nargs, NULL, kwnames, &_parser, 1, 1, 0, 1, varargssize, argsbuf);
if (!args) {
fastargs = _PyArg_UnpackKeywordsWithVarargFast(args, nargs, NULL, kwnames, &_parser, 1, 1, 0, 1, varargssize, argsbuf);
if (!fastargs) {
goto exit;
}
a = args[0];
__clinic_args = (PyObject *const *)args[1];
__clinic_args = args + 1;
if (!noptargs) {
goto skip_optional_kwonly;
}
b = PyObject_IsTrue(args[2]);
b = PyObject_IsTrue(fastargs[2]);
if (b < 0) {
goto exit;
}
Expand All @@ -3463,7 +3463,7 @@ static PyObject *
test_vararg_with_default_impl(PyObject *module, PyObject *a,
Py_ssize_t varargssize, PyObject *const *args,
int b)
/*[clinic end generated code: output=d38066fd9fa9336f input=6e110b54acd9b22d]*/
/*[clinic end generated code: output=51af6550b692f8d9 input=6e110b54acd9b22d]*/

/*[clinic input]
test_vararg_with_only_defaults
Expand Down Expand Up @@ -3496,29 +3496,30 @@ test_vararg_with_only_defaults(PyObject *module, PyObject *const *args, Py_ssize
static _PyArg_Parser _parser = {NULL, _keywords, "test_vararg_with_only_defaults", 0};
PyObject *argsbuf[3];
Py_ssize_t varargssize = Py_MAX(nargs - 0, 0);
PyObject *const *fastargs;
Py_ssize_t noptargs = 0 + (kwnames ? PyTuple_GET_SIZE(kwnames) : 0) - 0;
PyObject *const *__clinic_args = NULL;
PyObject *const *__clinic_args;
int b = 0;
PyObject *c = " ";

args = _PyArg_UnpackKeywordsWithVarargFast(args, nargs, NULL, kwnames, &_parser, 0, 0, 0, 0, varargssize, argsbuf);
if (!args) {
fastargs = _PyArg_UnpackKeywordsWithVarargFast(args, nargs, NULL, kwnames, &_parser, 0, 0, 0, 0, varargssize, argsbuf);
if (!fastargs) {
goto exit;
}
__clinic_args = (PyObject *const *)args[0];
__clinic_args = args + 0;
if (!noptargs) {
goto skip_optional_kwonly;
}
if (args[1]) {
b = PyObject_IsTrue(args[1]);
if (fastargs[1]) {
b = PyObject_IsTrue(fastargs[1]);
if (b < 0) {
goto exit;
}
if (!--noptargs) {
goto skip_optional_kwonly;
}
}
c = args[2];
c = fastargs[2];
skip_optional_kwonly:
return_value = test_vararg_with_only_defaults_impl(module, varargssize, __clinic_args, b, c);

Expand All @@ -3530,4 +3531,4 @@ static PyObject *
test_vararg_with_only_defaults_impl(PyObject *module, Py_ssize_t varargssize,
PyObject *const *args, int b,
PyObject *c)
/*[clinic end generated code: output=833c72a533c80a0e input=fa56a709a035666e]*/
/*[clinic end generated code: output=b2a08b5cd2854742 input=fa56a709a035666e]*/
25 changes: 13 additions & 12 deletions Python/clinic/bltinmodule.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 3 additions & 15 deletions Python/getargs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2690,14 +2690,6 @@ _PyArg_UnpackKeywordsWithVarargFast(PyObject *const *args, Py_ssize_t nargs,
return NULL;
}

/* pass varargs by pointer */
buf[vararg] = (PyObject *)&args[vararg];

/* copy required positional args */
for (i = 0; i < vararg; i++) {
buf[i] = args[i];
}

/* copy keyword args using kwtuple to drive process */
for (i = Py_MAX((int)nargs, posonly) -
Py_SAFE_DOWNCAST(varargssize, Py_ssize_t, int); i < maxargs; i++) {
Expand All @@ -2706,7 +2698,7 @@ _PyArg_UnpackKeywordsWithVarargFast(PyObject *const *args, Py_ssize_t nargs,
if (kwargs != NULL) {
current_arg = PyDict_GetItemWithError(kwargs, keyword);
if (!current_arg && PyErr_Occurred()) {
goto exit;
return NULL;
}
}
else {
Expand All @@ -2730,7 +2722,7 @@ _PyArg_UnpackKeywordsWithVarargFast(PyObject *const *args, Py_ssize_t nargs,
(parser->fname == NULL) ? "function" : parser->fname,
(parser->fname == NULL) ? "" : "()",
keyword, i+1);
goto exit;
return NULL;
}
}

Expand Down Expand Up @@ -2761,16 +2753,12 @@ _PyArg_UnpackKeywordsWithVarargFast(PyObject *const *args, Py_ssize_t nargs,
(parser->fname == NULL) ? "this function" : parser->fname,
(parser->fname == NULL) ? "" : "()");
}
goto exit;
return NULL;
}
}
}

return buf;

exit:
Py_XDECREF(buf[vararg]);
return NULL;
}

static const char *
Expand Down
66 changes: 37 additions & 29 deletions Tools/clinic/clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -862,6 +862,10 @@ def parser_body(prototype, *fields, declarations=''):
nargs = 'PyTuple_GET_SIZE(args)'
argname_fmt = 'PyTuple_GET_ITEM(args, %d)'

if vararg != NO_VARARG:
declarations = "Py_ssize_t varargssize = Py_MAX(nargs - %d, 0);" % (max_pos)
else:
declarations = ""

left_args = "{} - {}".format(nargs, max_pos)
max_args = NO_VARARG if (vararg != NO_VARARG) else max_pos
Expand All @@ -879,24 +883,17 @@ def parser_body(prototype, *fields, declarations=''):
if p.is_vararg():
if not new_or_init:
parser_code.append(normalize_snippet("""
%s = PyTuple_New(%s);
for (Py_ssize_t i = 0; i < %s; ++i) {{
PyTuple_SET_ITEM(%s, i, args[%d + i]);
}}
%s = %s + %d;
""" % (
p.converter.parser_name,
left_args,
left_args,
p.converter.parser_name,
max_pos
), indent=4))
p.converter.parser_name,
p.name,

@skirpichev skirpichev Feb 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would expect here p.converter.name instead.

I'm trying your patch for converting some vararg functions in the math module (see the old conversion attempt), and it seems that without this change autogenerated code is broken, because it references a Python parameter name instead of C (i.e., coordinates, instead of args):

static PyObject *
math_hypot_impl(PyObject *module, Py_ssize_t varargssize,
                PyObject *const *args);

static PyObject *
math_hypot(PyObject *module, PyObject *const *args, Py_ssize_t nargs)
{
    PyObject *return_value = NULL;
    Py_ssize_t varargssize = nargs;
    PyObject *const *__clinic_args;

    if (!_PyArg_CheckPositional("hypot", nargs, 0, PY_SSIZE_T_MAX)) {
        goto exit;
    }
    __clinic_args = coordinates + 0;
    return_value = math_hypot_impl(module, varargssize, __clinic_args);

exit:
    return return_value;
}

See docs. BTW, I think there should be a test for that.

@skirpichev skirpichev Feb 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hardcoding "args" will work too, but I'm not sure it's a good idea.

@colorfulappl colorfulappl Feb 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the advice.
When the function signature is METH_FASTCALL, the pass-in parameter is always named args, so replacing p.name with args works.
I made a fix and added two tests. It looks correct now.

vararg
), indent=4))
else:
# return a borrowed reference of items in args tuple
parser_code.append(normalize_snippet("""
%s = PyTuple_GetSlice(%d, -1);
""" % (
p.converter.parser_name,
max_pos
), indent=4))
%s = _PyTuple_CAST(args)->ob_item;
""" % p.converter.parser_name, indent=4))
continue

parsearg = p.converter.parse_arg(argname, displayname)
Expand Down Expand Up @@ -931,7 +928,7 @@ def parser_body(prototype, *fields, declarations=''):
goto exit;
}}
""", indent=4)]
parser_definition = parser_body(parser_prototype, *parser_code)
parser_definition = parser_body(parser_prototype, *parser_code, declarations=declarations)

else:
has_optional_kw = (max(pos_only, min_pos) + min_kw_only < len(converters))
Expand All @@ -951,23 +948,27 @@ def parser_body(prototype, *fields, declarations=''):
if not new_or_init:
flags = "METH_FASTCALL|METH_KEYWORDS"
parser_prototype = parser_prototype_fastcall_keywords
argname_fmt = 'args[%d]'
declarations = normalize_snippet("""
static const char * const _keywords[] = {{{keywords} NULL}};
static _PyArg_Parser _parser = {{NULL, _keywords, "{name}", 0}};
PyObject *argsbuf[%s];
""" % len(converters))
if vararg != NO_VARARG:
declarations += "\nPy_ssize_t varargssize = Py_MAX(nargs - %d, 0);" % (max_pos)
parsed_argname = "fastargs"
declarations += "\nPyObject *const *fastargs;"
else:
parsed_argname = "args"
if has_optional_kw:
pre_buffer = "0" if vararg != NO_VARARG else "nargs"
declarations += "\nPy_ssize_t noptargs = %s + (kwnames ? PyTuple_GET_SIZE(kwnames) : 0) - %d;" % (pre_buffer, min_pos + min_kw_only)
parser_code = [normalize_snippet("""
args = %s(args, nargs, NULL, kwnames, &_parser, %s, argsbuf);
if (!args) {{
parser_code = [normalize_snippet(f"""
{parsed_argname} = %s(args, nargs, NULL, kwnames, &_parser, %s, argsbuf);
if (!{parsed_argname}) {{{{
goto exit;
}}
}}}}
""" % args_declaration, indent=4)]
argname_fmt = f"{parsed_argname}[%d]"
else:
# positional-or-keyword arguments
flags = "METH_VARARGS|METH_KEYWORDS"
Expand All @@ -980,6 +981,8 @@ def parser_body(prototype, *fields, declarations=''):
PyObject * const *fastargs;
Py_ssize_t nargs = PyTuple_GET_SIZE(args);
""" % len(converters))
if vararg != NO_VARARG:
declarations += "\nPy_ssize_t varargssize = Py_MAX(nargs - %d, 0);" % (max_pos)
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("""
Expand All @@ -996,9 +999,18 @@ def parser_body(prototype, *fields, declarations=''):
add_label = None
for i, p in enumerate(parameters):
displayname = p.get_displayname(i+1)
if p.is_vararg():
p.converter.type = 'PyObject *const *'
parsearg = p.converter.parse_arg(argname_fmt % i, displayname)
if vararg != NO_VARARG:
# positional args
if i < int(vararg):
parsearg = p.converter.parse_arg('args[%d]' % i, displayname)
# keyword args
elif i > int(vararg):
parsearg = p.converter.parse_arg('fastargs[%d]' % i, displayname)
# vararg
else:
parsearg = p.converter.parse_arg('args + %d' % vararg, p.converter.parser_name)
else:
parsearg = p.converter.parse_arg(argname_fmt % i, displayname)
if parsearg is None:
#print('Cannot convert %s %r for %s' % (p.converter.__class__.__name__, p.converter.format_unit, p.converter.name), file=sys.stderr)
parser_code = None
Expand Down Expand Up @@ -4534,11 +4546,7 @@ def state_parameter(self, line):
if not default:
if self.parameter_state == self.ps_optional:
fail("Can't have a parameter without a default (" + repr(parameter_name) + ")\nafter a parameter with a default!")
if is_vararg:
value = NULL
kwargs.setdefault('c_default', "NULL")
else:
value = unspecified
value = unspecified
if 'py_default' in kwargs:
fail("You can't specify py_default without specifying a default value!")
else:
Expand Down