gh-90350: Optimize builtin functions min() and max() by colorfulappl · Pull Request #30286 · python/cpython · GitHub

Builtin functions min() and max() are labeled as METH_VARARGS | METH_KEYWORDS, which use tp_call calling convention.
After changing their label to METH_FASTCALL | METH_KEYWORDS, they can be invoked by vectorcall.

This optimization simplifies parameter passing and avoids creation of temporary tuple while parsing arguments, brings about up to 200%+ speed up on microbenchmarks.

faster-cpython/ideas#199

https://bugs.python.org/issue46192

Issue: Optimize builtin functions min() and max() #90350

If we're touching these, it would make sense to convert them to Argument Clinic, now that *args support is implemented (9af34c9).

cpython/Python/bltinmodule.c

Lines 1818 to 1823 in 77195cd

    
           /* AC: cannot convert yet, waiting for *args support */ 
        
           static PyObject * 
        
           builtin_min(PyObject *self, PyObject *args, PyObject *kwds) 
        
           { 
        
               return min_max(args, kwds, Py_LT); 
        
           }

cpython/Python/bltinmodule.c

Lines 1835 to 1840 in 77195cd

    
           /* AC: cannot convert yet, waiting for *args support */ 
        
           static PyObject * 
        
           builtin_max(PyObject *self, PyObject *args, PyObject *kwds) 
        
           { 
        
               return min_max(args, kwds, Py_GT); 
        
           }

I wrote an "Argument Clinic" version here:
colorfulappl@29b9559

But seems it is not as fast as current "without Argument Clinic" version:
colorfulappl@a9413ab

Result of microbench:

code snippet	with AC	w/o AC
max(1, 2)	1.11x faster	3.25x faster
max([1, 2])	1.14x faster	1.41x faster
max((1, ), (2, ), key=lambda x: x[0])	1.89x faster	1.85x faster
max([(1, ), (2, )], key=lambda x: x[0])	1.73x faster	1.52x faster
max([], default=-1)	2.39x faster	2.61x faster
max(1, 2, 3, 4, 5)	1.02x faster	2.46x faster

On the most commonly used case, "max(a, b [, ...])", there is not noticeable speed up when we use AC, especially when multiple arguments are passed (the last case).

I noticed that in 9af34c9 , the varargs on stack are packed to tuple before passed to callee, and callee obtains each argument by access the tuple's elements.
This slows down the function invocation.

One goal of fastcall is to avoid the creation of a temporary tuple to pass positional arguments (https://bugs.python.org/issue29259).

IMHO, the process of pack/unpack arguments to a tuple is unnecessary in 9af34c9 .
Perhaps it's better to pass an Object * const * pointer which points to the first argument and a nargs integer to indicate how many positional arguments are passed when we are processing varargs.

I would have a try, then may open another issue if necessary.

IMHO, the process of pack/unpack arguments to a tuple is unnecessary in 9af34c9 . Perhaps it's better to pass an Object * const * pointer which points to the first argument and a nargs integer to indicate how many positional arguments are passed when we are processing varargs.

I would have a try, then may open another issue if necessary.

Yes, such a change demands a separate issue / PR. It would be nice if you could add proof-of-concept benchmark results when you present the idea in the new bpo issue. I can add the relevant core devs on the nosy list, if you want.

Keep in mind that readability and maintainability weigh very heavy when considering a PR; that also goes for optimisation changes.

I wrote an "Argument Clinic" version here: colorfulappl@29b9559

While that change certainly looks better, it is buggy; it does not heed the key function. With that in mind, the benchmarks you posted in #30286 (comment) are obviously wrong; the benchmarks with the key keyword are not correct.

I wonder if Argument Clinic Itself could be updated to convert *args to vectorcall/fastcall if some kind of flag appears in the AC declaration

*args support was added to AC in #18609

@isidentical any thoughts on removing the tuple creation in some cases, or with some flag?

I would have a try, then may open another issue if necessary.

I opened a new issue (gh-90370 1),
and made a PR (#30312).

edit by @erlend-aasland: convert bpo link to gh link ↩

the benchmarks you posted in #30286 (comment) are obviously wrong; the benchmarks with the key keyword are not correct.

Thanks for revising my mistake. I'v seen the new results after your amend.
Actually, when the key function is empty, the "AC version" runs faster.

@colorfulappl Is this PR still blocked by #90370?

Summary: Builtin functions min() and max() now use METH_FASTCALL upstream issue: python/cpython#90350 upstream PR: python/cpython#30286 upstream commit: python/cpython@0066ab5 Reviewed By: carljm Differential Revision: D52650071 fbshipit-source-id: 93971e865ab9515efc9771c58582f63d15e0342d

…30286) Builtin functions min() and max() now use METH_FASTCALL

Builtin functions min() and max() now use METH_FASTCALL

a9413ab

This comment has been minimized.

Sign in to view

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

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

colorfulappl changed the title bpo-46192: Builtin functions min() and max() now use METH_FASTCALL bpo-46192: Optimize builtin functions min() and max() Dec 29, 2021

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

AlexWaygood requested a review from serhiy-storchaka December 29, 2021 10:14

This comment was marked as off-topic.

Sign in to view

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

This comment was marked as outdated.

Sign in to view

colorfulappl mannequin mentioned this pull request Apr 30, 2022

Optimize builtin functions min() and max() #90350

Closed

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

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

larryhastings mentioned this pull request Apr 10, 2022

Argument Clinic should understand *args and **kwargs parameters #64490

Open

Merge branch 'main' into opt_minmax

34142bf

hauntsaninja changed the title bpo-46192: Optimize builtin functions min() and max() gh-90350: Optimize builtin functions min() and max() Feb 26, 2023

serhiy-storchaka added 3 commits December 11, 2023 19:19

Merge branch 'main' into opt_minmax

4dae576

Fix compiler warnings.

9852e27

Use _PyCFunction_CAST.

610c3d5

Add a NEWS entry.

3a0413e

serhiy-storchaka approved these changes Dec 11, 2023

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels Dec 11, 2023

serhiy-storchaka merged commit 0066ab5 into python:main Dec 11, 2023

bedevere-app Bot removed the awaiting merge label Dec 11, 2023

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-90350 : Optimize builtin functions min() and max() (pythonGH-…

b69fd78

…30286) Builtin functions min() and max() now use METH_FASTCALL

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-90350 : Optimize builtin functions min() and max() (pythonGH-…

c292b70

…30286) Builtin functions min() and max() now use METH_FASTCALL

gh-90350: Optimize builtin functions min() and max() by colorfulappl · Pull Request #30286 · python/cpython · GitHub

Repository navigation

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

gh-90350: Optimize builtin functions min() and max()#30286serhiy-storchaka merged 6 commits intopython:mainpython/cpython:mainfrom colorfulappl:opt_minmaxcolorfulappl/cpython:opt_minmaxCopy head branch name to clipboard

Conversation

colorfulappl commented Dec 29, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

erlend-aasland commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colorfulappl commented Dec 30, 2021

Uh oh!

colorfulappl commented Dec 30, 2021

Uh oh!

erlend-aasland commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sweeneyde commented Dec 30, 2021

Uh oh!

sweeneyde commented Dec 31, 2021

Uh oh!

This comment was marked as off-topic.

colorfulappl commented Dec 31, 2021 • edited by erlend-aasland Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

colorfulappl commented Dec 31, 2021

Uh oh!

This comment was marked as outdated.

Uh oh!

eendebakpt commented Jul 16, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

gh-90350: Optimize builtin functions min() and max()#30286
serhiy-storchaka merged 6 commits into
python:mainfrom
colorfulappl:opt_minmax

colorfulappl commented Dec 29, 2021 •
edited by bedevere-bot
Loading

erlend-aasland commented Dec 29, 2021 •
edited
Loading

erlend-aasland commented Dec 30, 2021 •
edited
Loading

erlend-aasland commented Dec 30, 2021 •
edited
Loading

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