-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unused. All uses of the lock variable are guarded by the …_NOGIL
define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of initial comments. I'm currently trying to test this.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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