Skip to content
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

Jump to bottom

Make weakref thread-safe without the GIL #111926

Open
Tracked by #108219
colesbury opened this issue Nov 9, 2023 · 7 comments
Open
Tracked by #108219

Make weakref thread-safe without the GIL #111926

colesbury opened this issue Nov 9, 2023 · 7 comments
Assignees
@corona10
Labels
3.13 new features, bugs and security fixes topic-free-threading type-feature A feature request or enhancement

Comments

@colesbury
Copy link
Contributor

colesbury commented Nov 9, 2023 •
edited by bedevere-app bot

Feature or enhancement

The current weakref implementation relies on the GIL for thread-safety.

The nogil-3.12 fork substantially modifies the weakref implementation. I think we can implement a simpler change in CPython 3.13 (main) now that all PyObject's have their own mutex (in --disable-gil builds).

Basic idea

Protect access to the weakrefs linked list using the mutex in the weakly referenced object. Use the critical section API.

Prior implementation: colesbury/nogil-3.12@0dddcb6f9d

Linked PRs
@colesbury colesbury added type-feature A feature request or enhancement 3.13 new features, bugs and security fixes topic-free-threading labels Nov 9, 2023
@colesbury colesbury mentioned this issue Nov 9, 2023
Open
@corona10
Copy link
Member

corona10 commented Nov 10, 2023 •
edited

Maybe we should check the performance impact the default build first.
I will try to investigate it.

@colesbury
Copy link
Contributor Author

The critical section APIs are no-ops on the default build:

cpython/Include/internal/pycore_critical_section.h

Lines 107 to 113 in 289af86

#else /* !Py_NOGIL */
// The critical section APIs are no-ops with the GIL.
# define Py_BEGIN_CRITICAL_SECTION(op)
# define Py_END_CRITICAL_SECTION()
# define Py_BEGIN_CRITICAL_SECTION2(a, b)
# define Py_END_CRITICAL_SECTION2()
#endif /* !Py_NOGIL */

@corona10
Copy link
Member

corona10 commented Nov 10, 2023 •
edited

I know that there is no op for the default build but the commit you suggested modify some internal structure, I just want to know ovehead from the structure changed, even I actually do not expect real overhead from some refactoring. (I didn't look at deeply now please let me know if I miss something)

@colesbury
Copy link
Contributor Author

@corona10 the commit I linked to (in nogil-3.12) is not how I intend to make weakref thread-safe in CPython 3.13. I wouldn't spend too much time investigating it because it's not going to make it's way into 3.13. I linked to it because it's sometimes helpful for me to look back at the nogil-3.12 implementation.

@corona10
Copy link
Member

By the way, I am working on it :)

@corona10 corona10 mentioned this issue Nov 17, 2023
Closed
corona10 added a commit to corona10/cpython that referenced this issue Nov 17, 2023
@corona10
pythongh-111926 Update _weakref to be threadsafe in --disable-gil build
df6d6c1
@bedevere-app bedevere-app bot mentioned this issue Nov 17, 2023
Merged
corona10 added a commit to corona10/cpython that referenced this issue Nov 17, 2023
@corona10
pythongh-111926 Update _weakref to be threadsafe in --disable-gil build
c929bc0
corona10 added a commit that referenced this issue Nov 18, 2023
@corona10
gh-111926: Update _weakref to be threadsafe in --disable-gil build (g…
0566ab9 
…h-112189)
corona10 added a commit to corona10/cpython that referenced this issue Nov 19, 2023
@corona10
pythongh-111926 Update _PyWeakref_IS_DEAD to be thread-safe
866af79
@bedevere-app bedevere-app bot mentioned this issue Nov 19, 2023
Merged
corona10 added a commit that referenced this issue Nov 19, 2023
@corona10
gh-111926: Update _PyWeakref_IS_DEAD to be thread-safe (gh-112267)
7c9f267
corona10 added a commit to corona10/cpython that referenced this issue Jan 1, 2024
@corona10
pythongh-111926: Set up basic sementics of weakref API for freethreading
a400a06
@bedevere-app bedevere-app bot mentioned this issue Jan 1, 2024
Merged
corona10 added a commit that referenced this issue Jan 3, 2024
@corona10 @colesbury
gh-111926: Set up basic sementics of weakref API for freethreading (g…
0c3455a 
…h-113621)

---------

Co-authored-by: Sam Gross <colesbury@gmail.com>
@erlend-aasland
Copy link
Contributor

Is there more work to be done here? If yes, could you add a list of actionable items?

@corona10
Copy link
Member

corona10 commented Jan 5, 2024 •
edited

Is there more work to be done here? If yes, could you add a list of actionable items?

I am still checking for the weakrefobject.c :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@corona10 corona10

Labels
3.13 new features, bugs and security fixes topic-free-threading type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@colesbury @corona10 @erlend-aasland