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

colorfulappl · 2021-12-31T10:07:12Z

When "Augument Clinic generated code" are parsing arguments, the args are packed to a tuple before passing to callee. This may be unnecessary.

Pass a raw pointer which points to on-stack varargs, and a varargssize integer to indicate how many varargs are passed, can save the time of tuple creation/destruction and value copy.

BPO-issue: bpo-46212

Issue: Avoid temporary varargs tuple creation in argument passing #90370

…linic

the-knights-who-say-ni · 2021-12-31T10:07:16Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@colorfulappl

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

…keep backward compatibility

…ithVarargFast

sweeneyde

Just a couple of comments here.

I wonder if it would be simpler to restrict the scope of such a change to only signatures like def f(*args, kw1=<default1>, kw2=<default2>, ...), like print()/min()/max() have.

sweeneyde · 2022-01-05T06:25:20Z

Lib/test/clinic.test

@@ -3341,16 +3342,16 @@ test_vararg_and_posonly(PyObject *module, PyObject *const *args, Py_ssize_t narg
    for (Py_ssize_t i = 0; i < nargs - 1; ++i) {
        PyTuple_SET_ITEM(__clinic_args, i, args[1 + i]);


This seems suspicious -- isn't __clinic_args a pointer-to-PyObject-pointer, not a pointer-to-tuple?

My fault. The tuple doesn't need to be created.

sweeneyde · 2022-01-05T06:28:57Z

Lib/test/clinic.test

    if (!args) {
        goto exit;
    }
    a = args[0];
-    __clinic_args = args[1];
-    return_value = test_vararg_impl(module, a, __clinic_args);
+    __clinic_args = (PyObject *const *)args[1];


It would be nice to avoid such unsafe casts. Maybe passing a PyObject *** into _PyArg_UnpackKeywordsWithVarargFast instead?

This copy and cast are unnecessary.
Maybe __clinic_args = args + 1; is enough, note the args refers to the 2nd argument of function test_vararg, assuming it's not assigned in line 3389.

sweeneyde · 2022-01-05T06:35:37Z

Python/getargs.c

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


Isn't it possible to avoid copying at all by directly passing some sub-buffer of the *args buffer into the ..._impl function?

You are right. None of the positional args or variable-length args need to be copied. I will let _PyArg_UnpackKeywordsWithVarargFast parse default and keyword args only.

isidentical · 2022-01-21T14:04:33Z

Thanks for the PR and benchmarks @colorfulappl, I am current blocked a bit but plan to review this soon!

colorfulappl · 2022-01-21T14:23:58Z

Thanks for the PR and benchmarks @colorfulappl, I am current blocked a bit but plan to review this soon!

Thank you @isidentical !
With @sweeneyde 's comment I have found that my patch is actually buggy.
I am writing a new patch but blocked by some affairs last a few days.
You can review after I push a new commit and ignore this version. Sorry :)

isidentical · 2022-01-21T14:25:04Z

Sure, then just ping me whenever the new patch ready and I'll try to take a look at it then!

# Conflicts: # Python/clinic/bltinmodule.c.h

colorfulappl · 2022-03-23T12:09:06Z

Sorry for such a long delay.

When generate parsing code for function with varargs, this PR:

Introduce a new function _PyArg_UnpackKeywordsWithVarargKwonly.
It's a copy of _PyArg_UnpackKeywordsWithVararg but copying of positional args and packing of varargs are deleted.
_PyArg_UnpackKeywordsWithVarargKwonly returns an array pointer of parsed keyword args only.
Positional args are not copied to argsbuf. argsbuf only saves parsed keyword args.
Pass varargsize and vararg array pointer to XXXX_impl, this avoid pack/unpack of varargs.

I'd really appreciate it if you could take a look and give me some advice @isidentical .

colorfulappl · 2022-03-23T12:24:47Z

Benchmark Result

Benchmark:
https://bugs.python.org/file50533

Environment:
OS: macOS 12.3
CPU: Apple M1 Pro
Compiler: Apple clang version 13.1.6
Build args: --enable-optimizations --with-lto

+-----------------------------------------------------+---------+-----------------------+
| Benchmark                                           | base    | opt                   |
+=====================================================+=========+=======================+
| print(a)                                            | 133 ns  | 126 ns: 1.06x faster  |
+-----------------------------------------------------+---------+-----------------------+
| print(a, b, c)                                      | 337 ns  | 326 ns: 1.04x faster  |
+-----------------------------------------------------+---------+-----------------------+
| print(a, b, c, *v)                                  | 679 ns  | 661 ns: 1.03x faster  |
+-----------------------------------------------------+---------+-----------------------+
| print(a, sep='', file=stdout)                       | 134 ns  | 125 ns: 1.07x faster  |
+-----------------------------------------------------+---------+-----------------------+
| print(a, sep='', flush=True, file=stdout)           | 571 ns  | 563 ns: 1.01x faster  |
+-----------------------------------------------------+---------+-----------------------+
| print(*v, sep='', flush=True, file=stdout)          | 833 ns  | 819 ns: 1.02x faster  |
+-----------------------------------------------------+---------+-----------------------+
| print(a, b, c, *v, sep='', flush=True, file=stdout) | 1.18 us | 1.16 us: 1.02x faster |
+-----------------------------------------------------+---------+-----------------------+
| Geometric mean                                      | (ref)   | 1.03x faster          |
+-----------------------------------------------------+---------+-----------------------+

colorfulappl · 2022-03-24T08:42:50Z

I found there are some bugs when argument clinic processes varargs and made a fix in #32092.
Maybe this patch should wait until these bugs are fixed?

erlend-aasland · 2023-02-04T08:53:13Z

Just a heads-up, @colorfulappl: we prefer keeping all of the PR history since it makes reviewing easier, so please do not force-push. (Also documented in the devguide.) If you need to sync with main, you can do a git fetch --all && git merge --no-ff upstream/main.

colorfulappl · 2023-02-07T05:29:50Z

we prefer keeping all of the PR history since it makes reviewing easier, so please do not force-push.

Thanks for the advice; I will pay attention to it.

I added some test cases for the class method __new__ (without actually creating the objects, only packing and returning the arguments). And I found that when the __new__ or __init__ function accepts a vararg only, the argument clinic generated code does not check keyword arguments.

e.g.

# Signature of this function:
#
# @classmethod
# _testclinic.TestClassVarargOnly.__new__(cls, *args)

test_class_new_vararg_only(kw=1)

# Here we expect an error like "TypeError: TestClassVarargOnly.__new__() got an unexpected keyword argument 'kw'",
# but nothing raised.

We may need to fix it in another issue.

skirpichev · 2023-02-08T01:24:50Z

Tools/clinic/clinic.py

-                                max_pos
-                            ), indent=4))
+                            p.converter.parser_name,
+                            p.name,


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.

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

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.

skirpichev · 2023-02-08T04:06:17Z

Lib/test/test_clinic.py

+    def test_vararg_with_default_with_other_name(self):
+        with self.assertRaises(TypeError):
+            ac_tester.vararg_with_default_with_other_name()
+        self.assertEqual(ac_tester.vararg_with_default_with_other_name(1, b=False), (1, (), False))


You could add an exception test with _b.

skirpichev · 2023-02-08T05:09:45Z

Not sure, if this is a good idea, but could you consider using nargs instead of varargsize for *_impl() functions? This will reduce needs for changing the body of converted function (take math_gcd as an example to convert from scratch).

colorfulappl · 2023-02-08T06:54:00Z

When "Augument Clinic generated code" are parsing arguments, the args are packed to a tuple before passing to callee. This may be unnecessary.

This patch intends to reduce the overhead on passing vararg from Argument Clinic generated code to user-implemented *.impl() function.

Before this patch, vararg arguments are packed to a tuple, *_impl() function fetches these arguments by access the tuple items. In such a case, the number of vararg is implied in the tuple object.
After this patch, a pointer which points to vararg array is passed to *_impl() function directly, so the varargssize is necessary to tell the user how many vararg arguments is received.

but could you consider using nargs instead of varargsize for *_impl() functions?

@skirpichev Do you mean pass both nargs and max_pos to *_impl() function and let the user to calculate the number of vararg arguments? This may be confusing for users, and violate the purpose of Argument Clinic (Argument Clinic How-To - Abstract):

Argument Clinic is a preprocessor for CPython C files. Its purpose is to automate all the boilerplate involved with writing argument parsing code for “builtins”.

skirpichev · 2023-02-08T06:58:22Z

Indeed, my suggestion only valid for only-vararg case (as gcd, lcm and so on). Not sure if this case worth a special treatment...

skirpichev · 2024-02-04T02:18:51Z

@colorfulappl, could you fix merge conflicts?

colorfulappl added 2 commits December 31, 2021 17:44

Avoid temporary varargs tuple creation in argument passing

b68176d

Optimize argument passing of builtin print() with modified Argument C…

43b9410

…linic

the-knights-who-say-ni added the CLA not signed label Dec 31, 2021

bedevere-bot added the awaiting review label Dec 31, 2021

colorfulappl mentioned this pull request Dec 31, 2021

gh-90350: Optimize builtin functions min() and max() #30286

Merged

AlexWaygood added the performance Performance or resource usage label Dec 31, 2021

Revert modification of PyAPI_FUNC _PyArg_UnpackKeywordsWithVararg to …

9b37a77

…keep backward compatibility

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jan 4, 2022

blurb-it bot and others added 2 commits January 4, 2022 08:09

📜🤖 Added by blurb_it.

46e4472

Revert _PyArg_UnpackKeywordsWithVararg and add _PyArg_UnpackKeywordsW…

e06f343

…ithVarargFast

sweeneyde reviewed Jan 5, 2022

View reviewed changes

isidentical self-requested a review January 21, 2022 14:04

colorfulappl added 7 commits February 22, 2022 19:54

Fix a bug which allows more than one varargs

d35410b

Do not copy posargs and vararg during argument parsing

0a9fe91

Rename _PyArg_UnpackKeywordsWithVarargFast, it returns kwargs only now

9188052

Check type of varargs when generating argument parser

d245ec0

Fix varargssize calculation in class init and add test cases

8d16685

Merge branch 'main' into opt_ac

f4213ea

# Conflicts: # Python/clinic/bltinmodule.c.h

Rerun make clinic

156f8d6

Edit documentation

c32aed4

serhiy-storchaka added the topic-argument-clinic label Apr 30, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

colorfulappl added 6 commits February 6, 2023 14:15

Fix varargssize in new_or_init

8f61f5e

Simplify the code a bit

1eff215

Optimize generated varargssize assignment

5baf0f5

Fix argument passing in new_or_init

30c0606

Add tests for class method __new__

d9e7408

Merge branch 'main' into opt_ac

0f04a95

This comment was marked as outdated.

Sign in to view

arhadthedev mentioned this pull request Feb 7, 2023

doctest GitHub Actions job exits with 2 failing all recent PRs #101639

Closed

skirpichev requested changes Feb 8, 2023

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Feb 8, 2023

skirpichev mentioned this pull request Feb 8, 2023

invalid signature for math.hypot #101123

Closed

colorfulappl added 3 commits February 8, 2023 11:23

Correct test function name

85a8ac5

Fix vararg parsing when its name is not args

a9d1424

Merge branch 'main' into opt_ac

d6ff01a

skirpichev reviewed Feb 8, 2023

View reviewed changes

Add testcases for undeclared keyword arguments

597152e

erlend-aasland changed the title ~~bpo-46212: Avoid temporary varargs tuple creation in argument passing~~ gh-90370: Avoid temporary varargs tuple creation in argument passing Feb 27, 2023

skirpichev mentioned this pull request Mar 20, 2023

Speedup math.log by removing AC stuff #102839

Closed

Merge branch 'main' into opt_ac

3119fa6

colorfulappl mannequin mentioned this pull request Sep 7, 2022

Avoid temporary varargs tuple creation in argument passing #90370

Open

skirpichev added the stale Stale PR or inactive for long period of time. label Oct 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

colorfulappl commented Dec 31, 2021 •

edited by erlend-aasland

Loading

the-knights-who-say-ni commented Dec 31, 2021

sweeneyde left a comment

sweeneyde Jan 5, 2022

colorfulappl Jan 7, 2022

sweeneyde Jan 5, 2022

colorfulappl Jan 7, 2022

sweeneyde Jan 5, 2022

colorfulappl Jan 7, 2022

isidentical commented Jan 21, 2022

colorfulappl commented Jan 21, 2022

isidentical commented Jan 21, 2022

colorfulappl commented Mar 23, 2022

colorfulappl commented Mar 23, 2022

colorfulappl commented Mar 24, 2022

erlend-aasland commented Feb 4, 2023 •

edited

Loading

This comment was marked as outdated.

colorfulappl commented Feb 7, 2023

This comment was marked as outdated.

This comment was marked as outdated.

skirpichev Feb 8, 2023

skirpichev Feb 8, 2023

colorfulappl Feb 8, 2023

skirpichev Feb 8, 2023

skirpichev commented Feb 8, 2023

colorfulappl commented Feb 8, 2023

skirpichev commented Feb 8, 2023

skirpichev commented Feb 4, 2024

		@@ -3341,16 +3342,16 @@ test_vararg_and_posonly(PyObject module, PyObject const *args, Py_ssize_t narg
		for (Py_ssize_t i = 0; i < nargs - 1; ++i) {
		PyTuple_SET_ITEM(__clinic_args, i, args[1 + i]);

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

Are you sure you want to change the base?

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

Conversation

colorfulappl commented Dec 31, 2021 • edited by erlend-aasland Loading

the-knights-who-say-ni commented Dec 31, 2021

CLA Missing

sweeneyde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isidentical commented Jan 21, 2022

colorfulappl commented Jan 21, 2022

isidentical commented Jan 21, 2022

colorfulappl commented Mar 23, 2022

colorfulappl commented Mar 23, 2022

colorfulappl commented Mar 24, 2022

erlend-aasland commented Feb 4, 2023 • edited Loading

This comment was marked as outdated.

colorfulappl commented Feb 7, 2023

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skirpichev commented Feb 8, 2023

colorfulappl commented Feb 8, 2023

skirpichev commented Feb 8, 2023

skirpichev commented Feb 4, 2024

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

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

colorfulappl commented Dec 31, 2021 •

edited by erlend-aasland

Loading

erlend-aasland commented Feb 4, 2023 •

edited

Loading