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

[colorfulappl]

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

[erlend-aasland]

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);
	}

[colorfulappl]

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.

[colorfulappl]

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.

[erlend-aasland]

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.

[erlend-aasland]

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.

[sweeneyde]

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

[sweeneyde]

*args support was added to AC in #18609

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

[colorfulappl]

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

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

Footnotes

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

[colorfulappl]

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.

[eendebakpt]

@colorfulappl Is this PR still blocked by #90370?