Repository: python/cpython
Pull request: gh-134637: make PyCFuncPtr_call lock free in ctypes
<!--
Thanks for your contribution!
Please read this comment in its entirety. It's quite important.

# Pull Request title

It should be in the following format:

```
gh-NNNNNN: Summary of the changes made
```

Where: gh-NNNNNN refers to the GitHub issue number.

Most PRs will require an issue number. Trivial changes, like fixing a typo, do not need an issue.

# Backport Pull Request title

If this is a backport PR (PR made against branches other than `main`),
please ensure that the PR title is in the following format:

```
[X.Y] <title from the original PR> (GH-NNNNNN)
```

Where: [X.Y] is the branch name, for example: [3.13].

GH-NNNNNN refers to the PR number from `main`.

-->

This fixes the performance regression mentioned on the issue, it avoids acquiring the critical section in the general case of no contention.



<!-- gh-issue-number: gh-134637 -->
* Issue: gh-134637
<!-- /gh-issue-number -->


Problem statement:
3.14t vs 3.13t cuts IOCP performance in half
# Bug report

### Bug description:

This is about 3.14.0bl vs 3.13.1, free threaded in both cases.
Microsoft Windows [Version 10.0.19045.4529]

I run and maintain an IOCP server in python 3.13.1t.
There are no 3rd party libraries being used.

The Problem: Using the exact same code, running 3.14t vs 3.13t cuts the throughput in half.

I've made a badly written, working benchmark, extracted/simplified from my IOCP server.

Server:
```python
from ctypes import windll,create_string_buffer,c_void_p,c_ulong,c_ulonglong,Structure,byref,cast,addressof,POINTER,c_char
from ctypes.wintypes import DWORD,HANDLE

kernel32 = windll.kernel32

CreateNamedPipeW = kernel32.CreateNamedPipeW
CreateIOCompletionPort = kernel32.CreateIoCompletionPort
ConnectNamedPipe = kernel32.ConnectNamedPipe
GetQueuedCompletionStatusEx = kernel32.GetQueuedCompletionStatusEx;
ReadFile = kernel32.ReadFile

GLE = kernel32.GetLastError

class OVERLAPPED(Structure):
	_fields_ = (("0", c_void_p),("1", c_void_p),("2", DWORD),("3", DWORD),("4", c_void_p),
				("5", c_void_p),("6",c_void_p),("7",c_void_p),("8",c_void_p))


Overlapped = (OVERLAPPED*10)()
__Overlapped = byref(Overlapped)

IOCP = CreateIOCompletionPort(HANDLE(-1),None,0,4)

flag1 = 1 | 1073741824; flag2 = 4 | 2 | 0 | 8
Pipe = CreateNamedPipeW("\\\\.\\pipe\\IOCPBenchMark",flag1,flag2,255,32,0,0, None)

if not CreateIOCompletionPort(Pipe,IOCP,1,0): print("ERROR!")

ReadBuffer = create_string_buffer(1024)
__ReadBuffer = byref(ReadBuffer)

OverlapEntries = create_string_buffer(32*128)
ove = byref(OverlapEntries); 

Completed = c_ulong(0)
__Completed = byref(Completed)

def __IOCPThread():
	while True:
		while not GetQueuedCompletionStatusEx(IOCP, ove, 255, __Completed, 0, False): continue
		ReadFile(Pipe, __ReadBuffer,32,None,__Overlapped)


from threading import Thread

Threads = []
for t in range(4): Threads.append(Thread(target=__IOCPThread))


success = ConnectNamedPipe(Pipe, __Overlapped)
if not success:
	if GLE() != 997:
		print("ERROR 2")


while not GetQueuedCompletionStatusEx(IOCP, ove, 255, __Completed, 1, False): continue
print("Connected.")

ReadFile(Pipe, __ReadBuffer,32,None,__Overlapped)
for t in Threads: t.start()


from time import sleep
while True:
	sleep(1)
```


Client:
```python
from ctypes import windll,c_char_p,byref
from ctypes.wintypes import DWORD

from time import perf_counter as pfc

kernel32 = windll.kernel32

CreateFileW = kernel32.CreateFileW
WriteFile = kernel32.WriteFile
GLE = kernel32.GetLastError

written = DWORD()
__written = byref(written)


print(GLE())

GENERIC_WRITE = 1073741824
Pipe = kernel32.CreateFileW("\\\\.\\pipe\\IOCPBenchMark",GENERIC_WRITE,0,None,3,0,None)

if GLE() == 0: print("Connected.")

test = b"test"

t = pfc()+1
while True:

	for Count in range(1000000):

		if not WriteFile(Pipe, test, 4,__written, None):
			print("ERROR ",GLE())

		if not WriteFile(Pipe, test, 4,__written, None):
			print("ERROR ",GLE())

		if not WriteFile(Pipe, test, 4,__written, None):
			print("ERROR ",GLE())

		if not WriteFile(Pipe, test, 4,__written, None):
			print("ERROR ",GLE())

		if pfc() >= t:
			t = pfc()+1
			print(Count*4)
			break

```

The server uses 4 threads. if you don't see any output, try reducing the amount.
When I use 8 threads (on my 8 core machine), I don't get any output. d'uh.
No, SMT-Threads don't count for anything here.

Each script runs in its own cmd.exe window.
Please be aware that you'll have to kill the server-process manually.

I wanted to add a call to "taskkill/F /IM:pytho*",
but then realized I might cause someone big trouble with that.

`>python3.13t server.py`:
Client output:
205536
207128
206764
206504
204768


`>python3.14t server.py`:
Client output:
107468
105516
106032
107492
108472

Perplexity suggested I should post this here,
because this is a use-case you people might be interested in.

Thank you.

### CPython versions tested on:

3.14

### Operating systems tested on:

Windows

<!-- gh-linked-prs -->
### Linked PRs
* gh-134702
* gh-134742
<!-- /gh-linked-prs -->

Hints from issue discussion:
In 3.14, we made ctypes thread-safe, so this is probably the result of lock contention. I'm honestly surprised it's not crashing in 3.13. Would you mind benchmarking to find which lock is causing the problem?

cc @kumaraditya303
I think it is because  of using critical section around `PyCFuncPtr_call` because `restype` is mutable so it needs locking in `PyCFuncPtr_call`, I'll look into making it lock free. 
> In 3.14, we made ctypes thread-safe, so this is probably the result of lock contention. I'm honestly surprised it's not crashing in 3.13. Would you mind benchmarking to find which lock is causing the problem?
> 
> cc [@kumaraditya303](https://github.com/kumaraditya303)

Are you saying that my IOCP server, using 3.13t, shouldn't actually be working? Because it does. Flawlessly, even under load heavy enough to throttle the rest of the system.

Is there anything I can do to help?
> I think it is because of using critical section around PyCFuncPtr_call because restype is mutable so it needs locking in PyCFuncPtr_call, I'll look into making it lock free.

Oh, yeah, that sounds problematic. I'd be ok with removing that critical section entirely (or only holding it in places where we actually access per-object state), because it should up to the user to serialize their own C calls.

Commit message: add comments

Patch to review:
diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c
index 7de6bb396b084e..35931620f45937 100644
--- a/Modules/_ctypes/_ctypes.c
+++ b/Modules/_ctypes/_ctypes.c
@@ -3610,6 +3610,45 @@ generic_pycdata_new(ctypes_state *st,
   PyCFuncPtr_Type
 */
 
+static inline void
+atomic_xsetref(PyObject **field, PyObject *value)
+{
+#ifdef Py_GIL_DISABLED
+    PyObject *old = *field;
+    _Py_atomic_store_ptr(field, value);
+    Py_XDECREF(old);
+#else
+    Py_XSETREF(*field, value);
+#endif
+}
+/*
+    This function atomically loads the reference from *field, and
+    tries to get a new reference to it. If the incref fails,
+    it acquires critical section of obj and returns a new reference to the *field.
+    In the general case, this avoids contention on acquiring the critical section.
+*/
+static inline PyObject *
+atomic_xgetref(PyObject *obj, PyObject **field)
+{
+#ifdef Py_GIL_DISABLED
+    PyObject *value = _Py_atomic_load_ptr(field);
+    if (value == NULL) {
+        return NULL;
+    }
+    if (_Py_TryIncrefCompare(field, value)) {
+        return value;
+    }
+    Py_BEGIN_CRITICAL_SECTION(obj);
+    value = Py_XNewRef(*field);
+    Py_END_CRITICAL_SECTION();
+    return value;
+#else
+    return Py_XNewRef(*field);
+#endif
+}
+
+
+
 /*[clinic input]
 @critical_section
 @setter
@@ -3626,7 +3665,7 @@ _ctypes_CFuncPtr_errcheck_set_impl(PyCFuncPtrObject *self, PyObject *value)
         return -1;
     }
     Py_XINCREF(value);
-    Py_XSETREF(self->errcheck, value);
+    atomic_xsetref(&self->errcheck, value);
     return 0;
 }
 
@@ -3658,12 +3697,10 @@ static int
 _ctypes_CFuncPtr_restype_set_impl(PyCFuncPtrObject *self, PyObject *value)
 /*[clinic end generated code: output=0be0a086abbabf18 input=683c3bef4562ccc6]*/
 {
-    PyObject *checker, *oldchecker;
+    PyObject *checker;
     if (value == NULL) {
-        oldchecker = self->checker;
-        self->checker = NULL;
-        Py_CLEAR(self->restype);
-        Py_XDECREF(oldchecker);
+        atomic_xsetref(&self->restype, NULL);
+        atomic_xsetref(&self->checker, NULL);
         return 0;
     }
     ctypes_state *st = get_module_state_by_def(Py_TYPE(Py_TYPE(self)));
@@ -3679,11 +3716,9 @@ _ctypes_CFuncPtr_restype_set_impl(PyCFuncPtrObject *self, PyObject *value)
     if (PyObject_GetOptionalAttr(value, &_Py_ID(_check_retval_), &checker) < 0) {
         return -1;
     }
-    oldchecker = self->checker;
-    self->checker = checker;
     Py_INCREF(value);
-    Py_XSETREF(self->restype, value);
-    Py_XDECREF(oldchecker);
+    atomic_xsetref(&self->checker, checker);
+    atomic_xsetref(&self->restype, value);
     return 0;
 }
 
@@ -3728,16 +3763,16 @@ _ctypes_CFuncPtr_argtypes_set_impl(PyCFuncPtrObject *self, PyObject *value)
     PyObject *converters;
 
     if (value == NULL || value == Py_None) {
-        Py_CLEAR(self->converters);
-        Py_CLEAR(self->argtypes);
+        atomic_xsetref(&self->argtypes, NULL);
+        atomic_xsetref(&self->converters, NULL);
     } else {
         ctypes_state *st = get_module_state_by_def(Py_TYPE(Py_TYPE(self)));
         converters = converters_from_argtypes(st, value);
         if (!converters)
             return -1;
-        Py_XSETREF(self->converters, converters);
+        atomic_xsetref(&self->converters, converters);
         Py_INCREF(value);
-        Py_XSETREF(self->argtypes, value);
+        atomic_xsetref(&self->argtypes, value);
     }
     return 0;
 }
@@ -4533,9 +4568,8 @@ _build_result(PyObject *result, PyObject *callargs,
 }
 
 static PyObject *
-PyCFuncPtr_call_lock_held(PyObject *op, PyObject *inargs, PyObject *kwds)
+PyCFuncPtr_call(PyObject *op, PyObject *inargs, PyObject *kwds)
 {
-    _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op);
     PyObject *restype;
     PyObject *converters;
     PyObject *checker;
@@ -4560,13 +4594,24 @@ PyCFuncPtr_call_lock_held(PyObject *op, PyObject *inargs, PyObject *kwds)
     }
     assert(info); /* Cannot be NULL for PyCFuncPtrObject instances */
 
-    restype = self->restype ? self->restype : info->restype;
-    converters = self->converters ? self->converters : info->converters;
-    checker = self->checker ? self->checker : info->checker;
-    argtypes = self->argtypes ? self->argtypes : info->argtypes;
-/* later, we probably want to have an errcheck field in stginfo */
-    errcheck = self->errcheck /* ? self->errcheck : info->errcheck */;
-
+    restype = atomic_xgetref(op, &self->restype);
+    if (restype == NULL) {
+        restype = Py_XNewRef(info->restype);
+    }
+    converters = atomic_xgetref(op, &self->converters);
+    if (converters == NULL) {
+        converters = Py_XNewRef(info->converters);
+    }
+    checker = atomic_xgetref(op, &self->checker);
+    if (checker == NULL) {
+        checker = Py_XNewRef(info->checker);
+    }
+    argtypes = atomic_xgetref(op, &self->argtypes);
+    if (argtypes == NULL) {
+        argtypes = Py_XNewRef(info->argtypes);
+    }
+    /* later, we probably want to have an errcheck field in stginfo */
+    errcheck = atomic_xgetref(op, &self->errcheck);
 
     pProc = *(void **)self->b_ptr;
 #ifdef MS_WIN32
@@ -4575,11 +4620,21 @@ PyCFuncPtr_call_lock_held(PyObject *op, PyObject *inargs, PyObject *kwds)
         CDataObject *this;
         this = (CDataObject *)PyTuple_GetItem(inargs, 0); /* borrowed ref! */
         if (!this) {
+            Py_XDECREF(restype);
+            Py_XDECREF(converters);
+            Py_XDECREF(checker);
+            Py_XDECREF(argtypes);
+            Py_XDECREF(errcheck);
             PyErr_SetString(PyExc_ValueError,
                             "native com method call without 'this' parameter");
             return NULL;
         }
         if (!CDataObject_Check(st, this)) {
+            Py_XDECREF(restype);
+            Py_XDECREF(converters);
+            Py_XDECREF(checker);
+            Py_XDECREF(argtypes);
+            Py_XDECREF(errcheck);
             PyErr_SetString(PyExc_TypeError,
                             "Expected a COM this pointer as first argument");
             return NULL;
@@ -4587,12 +4642,22 @@ PyCFuncPtr_call_lock_held(PyObject *op, PyObject *inargs, PyObject *kwds)
         /* there should be more checks? No, in Python */
         /* First arg is a pointer to an interface instance */
         if (!this->b_ptr || *(void **)this->b_ptr == NULL) {
+            Py_XDECREF(restype);
+            Py_XDECREF(converters);
+            Py_XDECREF(checker);
+            Py_XDECREF(argtypes);
+            Py_XDECREF(errcheck);
             PyErr_SetString(PyExc_ValueError,
                             "NULL COM pointer access");
             return NULL;
         }
         piunk = *(IUnknown **)this->b_ptr;
         if (NULL == piunk->lpVtbl) {
+            Py_XDECREF(restype);
+            Py_XDECREF(converters);
+            Py_XDECREF(checker);
+            Py_XDECREF(argtypes);
+            Py_XDECREF(errcheck);
             PyErr_SetString(PyExc_ValueError,
                             "COM method call without VTable");
             return NULL;
@@ -4603,8 +4668,14 @@ PyCFuncPtr_call_lock_held(PyObject *op, PyObject *inargs, PyObject *kwds)
     callargs = _build_callargs(st, self, argtypes,
                                inargs, kwds,
                                &outmask, &inoutmask, &numretvals);
-    if (callargs == NULL)
+    if (callargs == NULL) {
+        Py_XDECREF(restype);
+        Py_XDECREF(converters);
+        Py_XDECREF(checker);
+        Py_XDECREF(argtypes);
+        Py_XDECREF(errcheck);
         return NULL;
+    }
 
     if (converters) {
         int required = Py_SAFE_DOWNCAST(PyTuple_GET_SIZE(converters),
@@ -4618,6 +4689,11 @@ PyCFuncPtr_call_lock_held(PyObject *op, PyObject *inargs, PyObject *kwds)
             */
             if (required > actual) {
                 Py_DECREF(callargs);
+                Py_XDECREF(restype);
+                Py_XDECREF(converters);
+                Py_XDECREF(checker);
+                Py_XDECREF(argtypes);
+                Py_XDECREF(errcheck);
                 PyErr_Format(PyExc_TypeError,
               "this function takes at least %d argument%s (%d given)",
                                  required,
@@ -4627,6 +4703,11 @@ PyCFuncPtr_call_lock_held(PyObject *op, PyObject *inargs, PyObject *kwds)
             }
         } else if (required != actual) {
             Py_DECREF(callargs);
+            Py_XDECREF(restype);
+            Py_XDECREF(converters);
+            Py_XDECREF(checker);
+            Py_XDECREF(argtypes);
+            Py_XDECREF(errcheck);
             PyErr_Format(PyExc_TypeError,
                  "this function takes %d argument%s (%d given)",
                      required,
@@ -4663,25 +4744,24 @@ PyCFuncPtr_call_lock_held(PyObject *op, PyObject *inargs, PyObject *kwds)
         if (v == NULL || v != callargs) {
             Py_DECREF(result);
             Py_DECREF(callargs);
+            Py_XDECREF(restype);
+            Py_XDECREF(converters);
+            Py_XDECREF(checker);
+            Py_XDECREF(argtypes);
+            Py_XDECREF(errcheck);
             return v;
         }
         Py_DECREF(v);
     }
-
+    Py_XDECREF(restype);
+    Py_XDECREF(converters);
+    Py_XDECREF(checker);
+    Py_XDECREF(argtypes);
+    Py_XDECREF(errcheck);
     return _build_result(result, callargs,
                          outmask, inoutmask, numretvals);
 }
 
-static PyObject *
-PyCFuncPtr_call(PyObject *op, PyObject *inargs, PyObject *kwds)
-{
-    PyObject *result;
-    Py_BEGIN_CRITICAL_SECTION(op);
-    result = PyCFuncPtr_call_lock_held(op, inargs, kwds);
-    Py_END_CRITICAL_SECTION();
-    return result;
-}
-
 static int
 PyCFuncPtr_traverse(PyObject *op, visitproc visit, void *arg)
 {


Code review comments:
diff_hunk: @@ -4575,24 +4620,44 @@ PyCFuncPtr_call_lock_held(PyObject *op, PyObject *inargs, PyObject *kwds)
         CDataObject *this;
         this = (CDataObject *)PyTuple_GetItem(inargs, 0); /* borrowed ref! */
         if (!this) {
+            Py_XDECREF(restype);
+            Py_XDECREF(converters);
+            Py_XDECREF(checker);
+            Py_XDECREF(argtypes);
+            Py_XDECREF(errcheck);
path: Modules/_ctypes/_ctypes.c
text: @user1:
Please switch to `goto error` or `goto finally` at this point.

@author:
I was avoiding that to keep the diff small for backporting, I have added it now.