Try to improve parallel in free-threading #6199
Conversation
Add locking to refnanny. Add mutex to guard the three exception state variables in a parallel block. Untested
Cython/Compiler/Nodes.py
Outdated
| c.putln("#else") | ||
| c.putln(f"int {Naming.parallel_freethreading_mutex}=0;") | ||
| c.putln(f"(void){Naming.parallel_freethreading_mutex};") |
There was a problem hiding this comment.
This seems unused. All uses of the lock variable are guarded by the …_NOGIL define.
There was a problem hiding this comment.
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)
|
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. |
Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
|
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. |
|
FWIW I agree we can merge this and test more intensively later, it certainly won't make things worse. |
|
I'm not sure there's an easy way of guarding that
Did you try if macros work in pragmas? Failing that, I'd also be ok with meeting this as is.
|
| @@ -9880,7 +9892,8 @@ def generate_execution_code(self, code): | |||
| if self.privates: | |||
| privates = [e.cname for e in self.privates | |||
| if not e.type.is_pyobject] | |||
| code.put('private(%s)' % ', '.join(sorted(privates))) | |||
| if privates: | |||
| code.put('private(%s)' % ', '.join(sorted(privates))) | |||
There was a problem hiding this comment.
I've snuck in a fix to #6203 here. I can take it out and do it separately if needed though.
Looks like macros work so I've done that now. |
| #include "internal/pycore_lock.h" | ||
| #endif | ||
|
|
||
| ////////////////////////// SharedInFreeThreading.proto ////////////////// |
There was a problem hiding this comment.
Regarding naming, since we settled on …_CPYTHON_NOGIL for the target macro, I'd prefer sticking to that consistently. Throwing "free threading" into the mix just makes the context less clear.
There was a problem hiding this comment.
I've done a quick fixup #6210.
Add locking to refnanny.
Add mutex to guard the three exception state variables in a parallel block.
Untested.
"Fixes" #6198