Avoid METH_VARARGS in clinic code

[nineteendo]

Feature or enhancement

Proposal:

Note: PR's for this should tackle a couple modules at a time.

We should avoid code like this in private clinic functions which aren't exported as public:

cpython/Modules/posixmodule.c

Lines 5470 to 5476 in 8fc953f

	/*[clinic input]
	os._path_normpath
	path: object
	Basic path normalization.
	[clinic start generated code]*/

This causes a lot of overhead with no practical benefit:

cpython/Modules/clinic/posixmodule.c.h

Lines 2257 to 2304 in 8fc953f

	#define OS__PATH_NORMPATH_METHODDEF \
	{"_path_normpath", _PyCFunction_CAST(os__path_normpath), METH_FASTCALL|METH_KEYWORDS, os__path_normpath__doc__},
	static PyObject *
	os__path_normpath_impl(PyObject *module, PyObject *path);
	static PyObject *
	os__path_normpath(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames)
	{
	PyObject *return_value = NULL;
	#if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE)
	#define NUM_KEYWORDS 1
	static struct {
	PyGC_Head _this_is_not_used;
	PyObject_VAR_HEAD
	PyObject *ob_item[NUM_KEYWORDS];
	} _kwtuple = {
	.ob_base = PyVarObject_HEAD_INIT(&PyTuple_Type, NUM_KEYWORDS)
	.ob_item = { &_Py_ID(path), },
	};
	#undef NUM_KEYWORDS
	#define KWTUPLE (&_kwtuple.ob_base.ob_base)
	#else // !Py_BUILD_CORE
	# define KWTUPLE NULL
	#endif // !Py_BUILD_CORE
	static const char * const _keywords[] = {"path", NULL};
	static _PyArg_Parser _parser = {
	.keywords = _keywords,
	.fname = "_path_normpath",
	.kwtuple = KWTUPLE,
	};
	#undef KWTUPLE
	PyObject *argsbuf[1];
	PyObject *path;
	args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, 1, 1, 0, argsbuf);
	if (!args) {
	goto exit;
	}
	path = args[0];
	return_value = os__path_normpath_impl(module, path);
	exit:
	return return_value;
	}

If we wrote this instead:

/*[clinic input]
os._path_normpath

    path: object
    /

Basic path normalization.
[clinic start generated code]*/

The overhead is gone:

#define OS__PATH_NORMPATH_METHODDEF    \
    {"_path_normpath", (PyCFunction)os__path_normpath, METH_O, os__path_normpath__doc__},

No, I didn't forget to include the rest of the code, this is all the code!

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

• Automate positional arguments in clinic code #117868

Linked PRs

• gh-117873: Deprecate grp keyword arguments #118010
• gh-117873: Use positional-only parameters in _posixshmem #118012
• gh-117873: Use positional-only parameters in os.path #118035
• gh-117873: Revert _posixshmem.shm_open() change #118901

[serhiy-storchaka]

Just yesterday I tried to write a patch which makes parameters of the C accelerators for os.path positional-only, to simplify and speed up the code. But then I realized that most of them replaces the Python implementations which support keyword arguments, therefore it would be a breaking change. So I abandoned that idea. But it would work for private helpers. Although the performance benefit is tiny in comparison of the overhead added by the Python wrapper.

Making parameters positional-only by default is an interesting idea, but it would require rewriting a lot of existing Argument Clinic declarations. It is difficult to do without manually, you have to write a conversion script. It would also differ from current defaults for Python function, it can cause different kind of errors. So I think that it is easier to just be careful when write new Argument Clinic declarations and don't add keyword support without need.

[erlend-aasland]

@serhiy-storchaka: one thought could be to have Argument Clinic emit a warning for varargs. (I get the feeling we've discussed this before.)

[vstinner]

Currently, you can use git grep to detect usage of METH_VARARGS. I'm not sure that a warning is worth it for all of these usage. A big part comes from _curses, likely because of the really weird "option groups". Another group comes from extensions built with the limited C API.

$ git grep METH_VARARGS $(find -name "*.c.h")
Modules/_multiprocessing/clinic/posixshmem.c.h:    {"shm_open", (PyCFunction)(void(*)(void))_posixshmem_shm_open, METH_VARARGS|METH_KEYWORDS, _posixshmem_shm_open__doc__},
Modules/_multiprocessing/clinic/posixshmem.c.h:    {"shm_unlink", (PyCFunction)(void(*)(void))_posixshmem_shm_unlink, METH_VARARGS|METH_KEYWORDS, _posixshmem_shm_unlink__doc__},
Modules/clinic/_cursesmodule.c.h:    {"addch", (PyCFunction)_curses_window_addch, METH_VARARGS, _curses_window_addch__doc__},
Modules/clinic/_cursesmodule.c.h:    {"addstr", (PyCFunction)_curses_window_addstr, METH_VARARGS, _curses_window_addstr__doc__},
Modules/clinic/_cursesmodule.c.h:    {"addnstr", (PyCFunction)_curses_window_addnstr, METH_VARARGS, _curses_window_addnstr__doc__},
Modules/clinic/_cursesmodule.c.h:    {"box", (PyCFunction)_curses_window_box, METH_VARARGS, _curses_window_box__doc__},
Modules/clinic/_cursesmodule.c.h:    {"delch", (PyCFunction)_curses_window_delch, METH_VARARGS, _curses_window_delch__doc__},
Modules/clinic/_cursesmodule.c.h:    {"derwin", (PyCFunction)_curses_window_derwin, METH_VARARGS, _curses_window_derwin__doc__},
Modules/clinic/_cursesmodule.c.h:    {"getch", (PyCFunction)_curses_window_getch, METH_VARARGS, _curses_window_getch__doc__},
Modules/clinic/_cursesmodule.c.h:    {"getkey", (PyCFunction)_curses_window_getkey, METH_VARARGS, _curses_window_getkey__doc__},
Modules/clinic/_cursesmodule.c.h:    {"get_wch", (PyCFunction)_curses_window_get_wch, METH_VARARGS, _curses_window_get_wch__doc__},
Modules/clinic/_cursesmodule.c.h:    {"hline", (PyCFunction)_curses_window_hline, METH_VARARGS, _curses_window_hline__doc__},
Modules/clinic/_cursesmodule.c.h:    {"insch", (PyCFunction)_curses_window_insch, METH_VARARGS, _curses_window_insch__doc__},
Modules/clinic/_cursesmodule.c.h:    {"inch", (PyCFunction)_curses_window_inch, METH_VARARGS, _curses_window_inch__doc__},
Modules/clinic/_cursesmodule.c.h:    {"insstr", (PyCFunction)_curses_window_insstr, METH_VARARGS, _curses_window_insstr__doc__},
Modules/clinic/_cursesmodule.c.h:    {"insnstr", (PyCFunction)_curses_window_insnstr, METH_VARARGS, _curses_window_insnstr__doc__},
Modules/clinic/_cursesmodule.c.h:    {"noutrefresh", (PyCFunction)_curses_window_noutrefresh, METH_VARARGS, _curses_window_noutrefresh__doc__},
Modules/clinic/_cursesmodule.c.h:    {"overlay", (PyCFunction)_curses_window_overlay, METH_VARARGS, _curses_window_overlay__doc__},
Modules/clinic/_cursesmodule.c.h:    {"overwrite", (PyCFunction)_curses_window_overwrite, METH_VARARGS, _curses_window_overwrite__doc__},
Modules/clinic/_cursesmodule.c.h:    {"refresh", (PyCFunction)_curses_window_refresh, METH_VARARGS, _curses_window_refresh__doc__},
Modules/clinic/_cursesmodule.c.h:    {"subwin", (PyCFunction)_curses_window_subwin, METH_VARARGS, _curses_window_subwin__doc__},
Modules/clinic/_cursesmodule.c.h:    {"scroll", (PyCFunction)_curses_window_scroll, METH_VARARGS, _curses_window_scroll__doc__},
Modules/clinic/_cursesmodule.c.h:    {"touchline", (PyCFunction)_curses_window_touchline, METH_VARARGS, _curses_window_touchline__doc__},
Modules/clinic/_cursesmodule.c.h:    {"vline", (PyCFunction)_curses_window_vline, METH_VARARGS, _curses_window_vline__doc__},
Modules/clinic/_cursesmodule.c.h:    {"newwin", (PyCFunction)_curses_newwin, METH_VARARGS, _curses_newwin__doc__},
Modules/clinic/_ssl.c.h:    {"read", (PyCFunction)_ssl__SSLSocket_read, METH_VARARGS, _ssl__SSLSocket_read__doc__},
Modules/clinic/gcmodule.c.h:    {"set_threshold", (PyCFunction)gc_set_threshold, METH_VARARGS, gc_set_threshold__doc__},
Modules/clinic/grpmodule.c.h:    {"getgrgid", (PyCFunction)(void(*)(void))grp_getgrgid, METH_VARARGS|METH_KEYWORDS, grp_getgrgid__doc__},
Modules/clinic/grpmodule.c.h:    {"getgrnam", (PyCFunction)(void(*)(void))grp_getgrnam, METH_VARARGS|METH_KEYWORDS, grp_getgrnam__doc__},
Modules/clinic/syslogmodule.c.h:    {"syslog", (PyCFunction)syslog_syslog, METH_VARARGS, syslog_syslog__doc__},
PC/clinic/_testconsole.c.h:    {"write_input", (PyCFunction)(void(*)(void))_testconsole_write_input, METH_VARARGS|METH_KEYWORDS, _testconsole_write_input__doc__},
PC/clinic/_testconsole.c.h:    {"read_output", (PyCFunction)(void(*)(void))_testconsole_read_output, METH_VARARGS|METH_KEYWORDS, _testconsole_read_output__doc__},
PC/clinic/winsound.c.h:    {"PlaySound", (PyCFunction)(void(*)(void))winsound_PlaySound, METH_VARARGS|METH_KEYWORDS, winsound_PlaySound__doc__},
PC/clinic/winsound.c.h:    {"Beep", (PyCFunction)(void(*)(void))winsound_Beep, METH_VARARGS|METH_KEYWORDS, winsound_Beep__doc__},
PC/clinic/winsound.c.h:    {"MessageBeep", (PyCFunction)(void(*)(void))winsound_MessageBeep, METH_VARARGS|METH_KEYWORDS, winsound_MessageBeep__doc__},

[vstinner]

Modules/clinic/grpmodule.c.h:    {"getgrgid", (PyCFunction)(void(*)(void))grp_getgrgid, METH_VARARGS|METH_KEYWORDS, grp_getgrgid__doc__},
Modules/clinic/grpmodule.c.h:    {"getgrnam", (PyCFunction)(void(*)(void))grp_getgrnam, METH_VARARGS|METH_KEYWORDS, grp_getgrnam__doc__},

I wrote PR gh-118010 for the grp extension module.

[vstinner]

Modules/_multiprocessing/clinic/posixshmem.c.h:    {"shm_open", (PyCFunction)(void(*)(void))_posixshmem_shm_open, METH_VARARGS|METH_KEYWORDS, _posixshmem_shm_open__doc__},
Modules/_multiprocessing/clinic/posixshmem.c.h:    {"shm_unlink", (PyCFunction)(void(*)(void))_posixshmem_shm_unlink, METH_VARARGS|METH_KEYWORDS, _posixshmem_shm_unlink__doc__},

I wrote PR gh-118012 for the _posixshmem extension module.

[vstinner]

I used VS Code to detect the usage of METH_KEYWORDS in private functions, (but potentially exported as public).

METH_VARARGS is inefficient, but I don't see why do you want to avoid "METH_FASTCALL|METH_KEYWORDS". Accepting keywords has no cost if arguments are passed as keywords. Also, sometimes, passing keywords makes an API way better.

Replacing METH_VARARGS with METH_FASTCALL is a nice goal, but there are some limitation, Argument Clinic is quite complicated, especially when it comes to option groups.

[nineteendo]

Is there a difference in performance between METH_FASTCALL|METH_KEYWORDS & METH_O?

[erlend-aasland]

Is there a difference in performance between METH_FASTCALL|METH_KEYWORDS & METH_O?

See the C API docs. Very often you will find answers to your questions in the docs; if the docs are missing information, it is a documentation bug (and in that case, you can file a bug report so we can improve the docs).

[erlend-aasland]

@vstinner: I'm not sure we should add changes like #118010 and #118012 this late in the 3.13 alpha phase; the feature freeze is imminent. IMO, if we are to make any changes like the ones suggested in this issue, we should wait until after PyCon when 3.14 development opens up.

[nineteendo]

See the C API docs. Very often you will find answers to your questions in the docs; if the docs are missing information, it is a documentation bug (and in that case, you can file a bug report so we can improve the docs).

It doesn't say anything about performance, I'll file a bug report.

[vstinner]

@vstinner: I'm not sure we should add changes like #118010 and #118012 this late in the 3.13 alpha phase; the feature freeze is imminent. IMO, if we are to make any changes like the ones suggested in this issue, we should wait until after PyCon when 3.14 development opens up.

Ok, I converted my two PRs a draft until main becomes Python 3.14 (next month).

[nineteendo]

I'll close this. It would be better to optimise METH_VARARGS instead, like @corona10 mentioned. Is this an idea for faster-cpython?