Skip to content
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.

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

gh-90370: avoid temporary tuple creation for vararg in AC #126064

Merged
merged 9 commits into from
Oct 31, 2024

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Oct 28, 2024

Current patch partially address issue, working in case when all arguments either positional-only or vararg.

Objects/setobject.c and Modules/gcmodule.c adapted. This fixes slight performance regression for set methods, introduced by #115112, e.g.:

Benchmark ref patch
set().update(s1, s2) 354 ns 264 ns: 1.34x faster

Benchmark code:

import pyperf
s1, s2 = {1}, {2}
runner = pyperf.Runner()
runner.bench_func('set().update(s1, s2)', set().update, s1, s2)

Current patch partially address issue, working in case when all
arguments either positional-only or vararg.

This also converts gcd(), lcm() and hypot() functions of the math module
to the Argument Clinic.  Fix issue python#101123.

Objects/setobject.c and Modules/gcmodule.c adapted.  This fixes slight
performance regression for set methods, introduced by
python#115112:

| Benchmark            | ref    | patch                |
|----------------------|:------:|:--------------------:|
| set().update(s1, s2) | 354 ns | 264 ns: 1.34x faster |

Benchmark code:
```py
import pyperf
s1, s2 = {1}, {2}
runner = pyperf.Runner()
runner.bench_func('set().update(s1, s2)', set().update, s1, s2)
```
@erlend-aasland
Copy link
Contributor

This also converts gcd(), lcm() and hypot() functions of the math module to the Argument Clinic. Fix issue #101123.

I'd prefer if we could keep those changes separate. Let's nok mix features (adapt methods/functions to Argument Clinic) and performance improvements (avoid temporary tuple creation for varargs in Argument Clinic).

@skirpichev
Copy link
Member Author

skirpichev commented Oct 28, 2024

I'd prefer if we could keep those changes separate.

Ok, reverted. Edit: description adjusted.

Modules/gcmodule.c Outdated Show resolved Hide resolved
erlend-aasland

This comment was marked as outdated.

@erlend-aasland

This comment was marked as outdated.

@rhettinger rhettinger removed their request for review October 28, 2024 14:07
@erlend-aasland
Copy link
Contributor

Re f179557: yes, you want make clinic-tests 😎

@erlend-aasland
Copy link
Contributor

I'll let this sit a couple of days to give @pablogsal a chance to chime in on the gc module change.

@erlend-aasland erlend-aasland self-assigned this Oct 28, 2024
@erlend-aasland erlend-aasland linked an issue Oct 28, 2024 that may be closed by this pull request
@skirpichev
Copy link
Member Author

gc module change.

I think this one is mostly mechanical change. Temporary tuple creation was moved from AC-generated code to gc_get_referrers_impl/gc_get_referents_impl.

@erlend-aasland
Copy link
Contributor

I think this one is mostly mechanical change. Temporary tuple creation was moved from AC-generated code to gc_get_referrers_impl/gc_get_referents_impl.

Yes, but still it is good practice to give code owners a chance to have a look.

@erlend-aasland erlend-aasland merged commit 8c22eba into python:main Oct 31, 2024
50 checks passed
@skirpichev skirpichev deleted the ac-vararg-opt-90370 branch October 31, 2024 11:29
@skirpichev
Copy link
Member Author

The rest of changes (math module): #126235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid temporary varargs tuple creation in argument passing
2 participants