bpo-46212: 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.

https://bugs.python.org/issue46212

…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:53:08Z

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:53:08Z

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:53:08Z

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?

colorfulappl added 2 commits Dec 31, 2021

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

bpo-46192: Optimize builtin functions min() and max() #30286

Open

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 Jan 4, 2022

📜🤖 Added by blurb_it.

46e4472

Revert _PyArg_UnpackKeywordsWithVararg and add _PyArg_UnpackKeywordsW…

e06f343

…ithVarargFast

sweeneyde reviewed Jan 5, 2022

View changes

isidentical self-requested a review Jan 21, 2022

colorfulappl added 7 commits Feb 22, 2022

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 expert-argument-clinic label Apr 30, 2022

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

bpo-46212: Avoid temporary `varargs` tuple creation in argument passing #30312

bpo-46212: Avoid temporary `varargs` tuple creation in argument passing #30312

colorfulappl commented Dec 31, 2021 •

edited by bedevere-bot

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

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

bpo-46212: Avoid temporary varargs tuple creation in argument passing #30312

Are you sure you want to change the base?

bpo-46212: Avoid temporary varargs tuple creation in argument passing #30312

Conversation

colorfulappl commented Dec 31, 2021 • edited by bedevere-bot

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

CLA Missing

sweeneyde left a comment

sweeneyde Jan 5, 2022

Choose a reason for hiding this comment

colorfulappl Jan 7, 2022

Choose a reason for hiding this comment

sweeneyde Jan 5, 2022

Choose a reason for hiding this comment

colorfulappl Jan 7, 2022

Choose a reason for hiding this comment

sweeneyde Jan 5, 2022

Choose a reason for hiding this comment

colorfulappl Jan 7, 2022

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

bpo-46212: Avoid temporary `varargs` tuple creation in argument passing #30312

bpo-46212: Avoid temporary `varargs` tuple creation in argument passing #30312

colorfulappl commented Dec 31, 2021 •

edited by bedevere-bot