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

Atomic operations for only free-threaded builds #115041

Closed
mpage opened this issue Feb 5, 2024 · 2 comments
Closed

Atomic operations for only free-threaded builds #115041

mpage opened this issue Feb 5, 2024 · 2 comments
Labels
topic-free-threading type-feature A feature request or enhancement

Comments

@mpage
Copy link
Contributor

mpage commented Feb 5, 2024

Feature or enhancement

Proposal:

Based on discussion in #113830 and elsewhere, there are situations in which we need to use an atomic operation in free-threaded builds, but do not need to use one in the default build, and do not want to introduce the potential performance overhead of an atomic operation in the default build. Since we anticipate that these situations will be common, we'd like to introduce wrappers around the functions found in cpython/atomic.h that perform the operation atomically in free-threaded builds and use the non-atomic equivalent in default builds.

To get discussion started, I propose the following:

  1. We add a header that lives alongside cpython/pyatomic.h (pyatomic_free_threaded_wrappers.h?) that provides definitions of the wrappers.
  2. Wrappers look like:
static inline _Py_ssize_t
_Py_ft_only_atomic_load_ssize(Py_ssize_t *obj)
{
    #ifdef Py_GIL_DISABLED
        return *obj;
    #else
        return _Py_atomic_load_ssize(obj);
    #endif
}

Naming is hard. I'm not particularly in love with _ft_only_ and would love a better alternative to communicate that the operation is atomic in free-threaded builds and non-atomic in default builds.

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

@mpage mpage added the type-feature A feature request or enhancement label Feb 5, 2024
@mpage
Copy link
Contributor Author

mpage commented Feb 5, 2024

cc @corona10 @colesbury

@colesbury
Copy link
Contributor

colesbury commented Feb 5, 2024

This should be an internal-only header, so in Include/internal named pycore_xxx.h.

Let's try to keep this small and only add the functions we need instead of wrapping every function from pyatomic.h. There's a few places in dict and list (and your PR) where this will be useful, but I don't think it will be widely needed. I expect that many of the places that don't use critical sections, like PyDict_GetItemRef, will have different enough code paths for the free-threaded build that we will need to guard them with #ifdef Py_GIL_DISABLED rather than rely on these wrappers.

mpage added a commit to mpage/cpython that referenced this issue Feb 13, 2024
colesbury pushed a commit that referenced this issue Feb 14, 2024
…115046)

These are intended to be used in places where atomics are required in
free-threaded builds but not in the default build. We don't want to
introduce the potential performance overhead of an atomic operation in the
default build.
@mpage mpage closed this as completed Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-free-threading type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants