[cython/cython] Try to improve parallel in free-threading (PR #6199)

da-woods

unread,

May 14, 2024, 3:01:04 PM5/14/24

to cython/cython, Subscribed

Add locking to refnanny.

Add mutex to guard the three exception state variables in a parallel block.

Untested.

"Fixes" #6198

You can view, comment on, or merge this pull request online at:

https://github.com/cython/cython/pull/6199

Commit Summary

e1dfd35 Try to improve parallel in free-threading

File Changes

(5 files)

Patch Links:

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

scoder

unread,

May 15, 2024, 4:16:35 AM5/15/24

to cython/cython, Subscribed

@scoder commented on this pull request.

Looks good to me.

In Cython/Compiler/Nodes.py:

> +            c.putln("#else")
+            c.putln(f"int {Naming.parallel_freethreading_mutex}=0;")
+            c.putln(f"(void){Naming.parallel_freethreading_mutex};")

This seems unused. All uses of the lock variable are guarded by the …_NOGIL define.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

da-woods

unread,

May 15, 2024, 1:07:03 PM5/15/24

to cython/cython, Subscribed

@da-woods commented on this pull request.

In Cython/Compiler/Nodes.py:

> +            c.putln("#else")
+            c.putln(f"int {Naming.parallel_freethreading_mutex}=0;")
+            c.putln(f"(void){Naming.parallel_freethreading_mutex};")

It's declared as shared in the openmp pragma (and I'm not sure there's an easy way of guarding that) so I think this is used.

shared_vars.append(Naming.parallel_freethreading_mutex)

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

da-woods

unread,

May 15, 2024, 1:07:54 PM5/15/24

to cython/cython, Subscribed

I'll wait until either I've had time to set up a free-threading build and give it a try, or until someone with one reports back.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Lysandros Nikolaou

unread,

May 16, 2024, 9:05:33 AM5/16/24

to cython/cython, Subscribed

@lysnikolaou commented on this pull request.

A couple of initial comments. I'm currently trying to test this.

In Cython/Compiler/Nodes.py:

> @@ -9767,6 +9772,12 @@ def end_parallel_control_flow_block(
         if self.error_label_used:
             c.putln("const char *%s = NULL; int %s = 0, %s = 0;" % self.parallel_pos_info)
             c.putln("PyObject *%s = NULL, *%s = NULL, *%s = NULL;" % self.parallel_exc)
+            c.putln("#if CYTHON_COMPILING_IN_CPYTHON_NOGIL")
+            c.putln(f"PyMutex {Naming.parallel_freethreading_mutex};")

We need to initialize the mutex here, right?

⬇️ Suggested change

-            c.putln(f"PyMutex {Naming.parallel_freethreading_mutex};")
+            c.putln(f"PyMutex {Naming.parallel_freethreading_mutex} = {0};")

In Cython/Runtime/refnanny.pyx:

> +    #if CYTHON_COMPILING_IN_CPYTHON_NOGIL
+    static CYTHON_INLINE int __Pyx_refnanny_init_lock(PyThread_type_lock* lock) {
+         *lock = PyThread_allocate_lock();
+         if (!*lock) {
+            PyErr_NoMemory();
+            return -1;
+         }
+         return 0;
+    }
+
+    static CYTHON_INLINE void __Pyx_refnanny_free_lock(PyThread_type_lock lock) {
+        PyThread_free_lock(lock);
+    }
+
+    static CYTHON_INLINE void __Pyx_refnanny_lock_acquire(PyThread_type_lock lock) {
+        while (!PyThread_acquire_lock_timed(lock, 100, 0) {

⬇️ Suggested change

-        while (!PyThread_acquire_lock_timed(lock, 100, 0) {
+        while (!PyThread_acquire_lock_timed(lock, 100, 0)) {

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

da-woods

unread,

May 16, 2024, 3:12:08 PM5/16/24

to cython/cython, Push

@da-woods pushed 1 commit.

8f87903 Apply suggestions from code review

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Lysandros Nikolaou

unread,

May 16, 2024, 4:11:19 PM5/16/24

to cython/cython, Subscribed

@lysnikolaou commented on this pull request.

In Cython/Compiler/Nodes.py:

> @@ -9767,6 +9772,12 @@ def end_parallel_control_flow_block(
         if self.error_label_used:
             c.putln("const char *%s = NULL; int %s = 0, %s = 0;" % self.parallel_pos_info)
             c.putln("PyObject *%s = NULL, *%s = NULL, *%s = NULL;" % self.parallel_exc)
+            c.putln("#if CYTHON_COMPILING_IN_CPYTHON_NOGIL")
+            c.putln(f"PyMutex {Naming.parallel_freethreading_mutex} = {0};")

I forgot this was inside an f-string before. I'm sorry about that.

⬇️ Suggested change

-            c.putln(f"PyMutex {Naming.parallel_freethreading_mutex} = {0};")
+            c.putln(f"PyMutex {Naming.parallel_freethreading_mutex} = {{0}};")

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

da-woods

unread,

May 16, 2024, 4:12:19 PM5/16/24

to cython/cython, Push

@da-woods pushed 1 commit.

c4e9ad9 Update Cython/Compiler/Nodes.py

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

da-woods

unread,

May 19, 2024, 4:28:11 PM5/19/24

to cython/cython, Subscribed

I'll merge this in a couple of days if there aren't any further comments. I wasn't able to reproduce the bug it was allegedly supposed to be fixing. But I think we do want locking in these places, and I was able to test the PR and confirm that it doesn't seem to break anything.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Lysandros Nikolaou

unread,

May 21, 2024, 2:33:35 PM5/21/24

to cython/cython, Subscribed

FWIW I agree we can merge this and test more intensively later, it certainly won't make things worse.