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

Make set thread-safe in --disable-gil builds #112069

Closed
3 tasks done
Tracked by #108219
colesbury opened this issue Nov 14, 2023 · 7 comments
Closed
3 tasks done
Tracked by #108219

Make set thread-safe in --disable-gil builds #112069

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

Comments

@colesbury
Copy link
Contributor

colesbury commented Nov 14, 2023

Feature or enhancement

The set object is not currently thread-safe in --disable-gil builds. We should make it thread-safe by using the "critical section" API. to acquire the per-object locks around operations. There should be no effective change in the default build (i.e., with GIL) because critical sections are no-ops in the default build.

Notes:

  1. Unlike dict and list, I don't think it's worth the complexity to try to "optimistically avoid locking" around any set operation (except set_len). We could consider doing this in the future if there is a performance justification, but not for 3.13.
  2. set_len can avoid locking and instead use relaxed atomics for reading the "used" field. Note that writes to "used" should then also use relaxed atomics.
  3. Some operations require locking two containers (like set_merge). Some of these will need refactorings so that the critical sections macros can be added in the correct places.

For context, here is the change from the nogil-3.12 fork: colesbury/nogil-3.12@4ca2924f0d. Note that the critical section API is slightly changed in 3.13 from nogil-3.12; In 3.13 Py_BEGIN_CRITICAL_SECTION takes a PyObject instead of a PyMutex.

TODO:

Linked PRs

@tomasr8
Copy link
Contributor

tomasr8 commented Nov 16, 2023

Hi again! This one looks more challenging than the hashlib one but I'd like to try anyway :)

@colesbury
Copy link
Contributor Author

Thanks @tomasr8!

@DinoV
Copy link
Contributor

DinoV commented Jan 4, 2024

hi @tomasr8, just curious if you're still planning on working on this?

@tomasr8
Copy link
Contributor

tomasr8 commented Jan 5, 2024

Hi! Yes, I took a break for Christmas but I have it almost working, just need to fix some tests. I could open a draft PR this weekend

colesbury added a commit that referenced this issue Mar 8, 2024
This makes nearly all the operations on set thread-safe in the free-threaded build, with the exception of `_PySet_NextEntry` and `setiter_iternext`.

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
colesbury added a commit to colesbury/cpython that referenced this issue Mar 8, 2024
…ython#113800)"

The "AMD64 Debian root 3.x" is failing with strange errors.

This reverts commit c951e25.
adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 25, 2024
…113800)

This makes nearly all the operations on set thread-safe in the free-threaded build, with the exception of `_PySet_NextEntry` and `setiter_iternext`.

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
corona10 added a commit to corona10/cpython that referenced this issue Apr 16, 2024
@corona10
Copy link
Member

@colesbury I am going to work on _PySet_NextEntry this week, considering the caller-side usage.

@corona10 corona10 self-assigned this Apr 16, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
…113800)

This makes nearly all the operations on set thread-safe in the free-threaded build, with the exception of `_PySet_NextEntry` and `setiter_iternext`.

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
corona10 added a commit to corona10/cpython that referenced this issue Apr 17, 2024
corona10 added a commit to corona10/cpython that referenced this issue Apr 18, 2024
corona10 added a commit that referenced this issue Apr 18, 2024
@corona10
Copy link
Member

@colesbury Now we can close the issue right? Or more thing is left?

@colesbury
Copy link
Contributor Author

Yeah, I think so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes topic-free-threading type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants