gh-127065: Make methodcaller thread-safe and re-entrant by eendebakpt · Pull Request #127245 · python/cpython · GitHub

The function operator.methodcaller is not thread-safe since the additional of the vectorcall method in ##107201. In the free-threading build the issue is easy to trigger, for the normal build harder.

We could either remove the vectorcall implementation, or adapt the vectorcall implementation to be thread safe (for both the normal and free-threading build). In this PR we make the methodcaller safe by

Replacing the lazy initialiation with initialization in the constructor
Using a stack allocated space for the vectorcall arguments, and falling back to tp_call for more than 8 arguments

Benchmark results:

call: Mean +- std dev: [no_vectorcall] 353 ns +- 8 ns -> [pr] 149 ns +- 5 ns: 2.37x faster
creation: Mean +- std dev: [no_vectorcall] 317 ns +- 8 ns -> [pr] 439 ns +- 31 ns: 1.39x slower
creation+call: Mean +- std dev: [no_vectorcall] 652 ns +- 17 ns -> [pr] 613 ns +- 14 ns: 1.06x faster
call kwarg: Mean +- std dev: [no_vectorcall] 599 ns +- 88 ns -> [pr] 199 ns +- 6 ns: 3.01x faster
creation kwarg: Mean +- std dev: [no_vectorcall] 451 ns +- 4 ns -> [pr] 653 ns +- 59 ns: 1.45x slower
creation+call kwarg: Mean +- std dev: [no_vectorcall] 1.05 us +- 0.02 us -> [pr] 858 ns +- 9 ns: 1.22x faster

Geometric mean: 1.29x faster

(old machine, no PGO, comparing this PR with the same PR but the vectorcall disabled)

Benchmark script

import pyperf

setup = """
from operator import methodcaller as mc
arr = []
call = mc('sort')
call_kwarg = mc('sort', reverse=True)
"""

runner = pyperf.Runner()
runner.timeit(name="call", stmt="call(arr)", setup=setup)
runner.timeit(name="creation", stmt="call = mc('sort')", setup=setup)
runner.timeit(name="creation+call", stmt="call = mc('sort'); call(arr)", setup=setup)
runner.timeit(name="call kwarg", stmt="call_kwarg(arr)", setup=setup)
runner.timeit(name="creation kwarg", stmt="call = mc('sort', reverse=True)", setup=setup)
runner.timeit(name="creation+call kwarg", stmt="call = mc('sort', reverse=True); call(arr)", setup=setup)

Issue: methodcaller is not thread-safe (or re-entrant) #127065

A few comments:

I don't think _methodcaller_initialize_vectorcall is thread safe without the GIL. Lazily initializing arguments complicates the code and thread-safety -- I don't think it's worth it.
Generally, I think it's better to support some fixed max number of arguments (like 8), and stack allocate the temporary array. (i.e., PyObject **tmp_args[MAX_ARGS];). If the methodcaller needs more arguments than that, just don't set the vectorcall field and let it fall back to the tp_call.

PyMem_Free is after the return statement

…methodcaller_ft

The ownership here seems a bit complicated and I think it can be simplified. As I understand it, vectorcall_kwnames only exists to ensure that some entries in vectorcall_args stay alive.

Instead, I'd suggest:

Make PyObject *vectorcall_args a tuple (that holds strong references to its contents as usual)
Get rid of vectorcall_kwnames
Use _PyTuple_ITEMS for fast access to the contents of the tuple (for memcpy())
Visit vectorcall_args in methodcaller_traverse

The vectorcall_kwnames is needed as an argument for PyObject_VectorcallMethod in methodcaller_vectorcall (https://github.com/python/cpython/blob/main/Modules/_operator.c#L1666), so we cannot get rid of it.

The ownership is not too hard I think: the objects in vectorcall_args have references borrowed from either mc->args or (the keys from) mc->kwds. I added a comment to clarify this.

Making the vectorcall_args a tuple is still an option though. It requires a bit more memory and a tiny bit of computation in the initialization. It would be the C equivalent of vectorcall_args = args + tuple(kwds). I'll work it out in a branch to see how it compares

Okay, in that case don't worry about it unless you prefer it as a tuple.

The diff between the two approaches is this:

eendebakpt/cpython@methodcaller_ft...eendebakpt:cpython:methodcaller_ft_v2

What is nice about making vectorcall_args a tuple is that if there are no keyword arguments, we can reuse mc->args. It does require more operations in the construction though.

I think either approach is fine! My guess is that the vast majority of uses of methodcaller() do not use keyword arguments.

Running benchmarks shows what is to be expected from the implementations: using a tuple for vectorcall_args is a bit slower in initializing, except when there are no keyword arguments (since then we reuse the arg tuple). Differences are small though.

Since using a tuple leads to cleaner code and the majority of uses is without keywords I slightly prefer the tuple approach. I will open a new PR for it.

Overall this looks good to me. A few minor formatting suggestions below.

I think the more common formatting looks like:

methodcaller_vectorcall(methodcallerobject *mc, PyObject *const *args,
                        size_t nargsf, PyObject *kwnames)

Up to you, but I'd write this without the temporary result variable like:

return PyObject_VectorcallMethod(...);

Suggested change

      
            static int _methodcaller_initialize_vectorcall(methodcallerobject* mc)
          
            static int
          
            _methodcaller_initialize_vectorcall(methodcallerobject *mc)

Co-authored-by: Sam Gross <colesbury@gmail.com>

…methodcaller_ft

Closing in favor of #127746

eendebakpt added 3 commits November 23, 2024 21:04

Make methodcalled thread-safe

223d650

check result of PyMem_Malloc

b6d454a

enable ft

4ce1233

bedevere-app Bot mentioned this pull request Nov 25, 2024

methodcaller is not thread-safe (or re-entrant) #127065

Closed

bedevere-app Bot added the awaiting review label Nov 25, 2024

fix memory error

cf6b79b

colesbury reviewed Nov 25, 2024

View reviewed changes

eendebakpt and others added 11 commits November 28, 2024 22:38

wip

2476ce4

add tests

5ecf876

wip

709010d

wip

6bd2c2e

Merge branch 'main' into methodcaller_ft

8d40552

wip

c9e3898

fix memory error

8ef7a04

skip check on zero size for memcpy

b4f30d3

📜🤖 Added by blurb_it.

6d06201

Merge branch 'methodcaller_ft' of github.com:eendebakpt/cpython into …

56cdc1f

…methodcaller_ft

Merge branch 'main' into methodcaller_ft

440eb0c

eendebakpt changed the title Draft: gh-127065: Make methodcaller thread-safe and re-entrant gh-127065: Make methodcaller thread-safe and re-entrant Dec 1, 2024

colesbury reviewed Dec 2, 2024

View reviewed changes

eendebakpt added 2 commits December 3, 2024 20:45

review comments

ad66951

review comments

bc3fe2a

colesbury reviewed Dec 3, 2024

View reviewed changes

eendebakpt and others added 4 commits December 6, 2024 21:53

Update Modules/_operator.c

e9a1fa6

Co-authored-by: Sam Gross <colesbury@gmail.com>

Update Modules/_operator.c

00ab654

Co-authored-by: Sam Gross <colesbury@gmail.com>

review comments

5a7344b

Merge branch 'methodcaller_ft' of github.com:eendebakpt/cpython into …

f9f53fe

…methodcaller_ft

eendebakpt mentioned this pull request Dec 8, 2024

gh-127065: Make methodcaller thread-safe and re-entrant (v2) #127746

Merged

eendebakpt closed this Dec 11, 2024

                           (PyTuple_GET_SIZE(mc->xargs)) | PY_VECTORCALL_ARGUMENTS_OFFSET,
                           mc->vectorcall_kwnames);
+                  PyMem_Free(tmp_args);

		PyObject *vectorcall_args; / Borrowed references */
		PyObject *vectorcall_kwnames;

		methodcaller_vectorcall(
		methodcallerobject mc, PyObject const args, size_t nargsf, PyObject kwnames)

+                  assert(1 + number_of_arguments <= _METHODCALLER_MAX_ARGS);
+                  memcpy(tmp_args + 1, mc->vectorcall_args, sizeof(PyObject *) * number_of_arguments);
+                  PyObject *result = PyObject_VectorcallMethod(

	static int _methodcaller_initialize_vectorcall(methodcallerobject* mc)
	static int
	_methodcaller_initialize_vectorcall(methodcallerobject *mc)

gh-127065: Make methodcaller thread-safe and re-entrant by eendebakpt · Pull Request #127245 · python/cpython · GitHub

Repository navigation

gh-127065: Make methodcaller thread-safe and re-entrant - #127245

gh-127065: Make methodcaller thread-safe and re-entrant#127245eendebakpt wants to merge 21 commits intopython:mainpython/cpython:mainfrom eendebakpt:methodcaller_fteendebakpt/cpython:methodcaller_ftCopy head branch name to clipboard

Conversation

eendebakpt commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eendebakpt commented Dec 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gh-127065: Make methodcaller thread-safe and re-entrant#127245
eendebakpt wants to merge 21 commits into
python:mainfrom
eendebakpt:methodcaller_ft

eendebakpt commented Nov 25, 2024 •
edited
Loading