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

gh-115041: Add wrappers that are atomic only in free-threaded builds #115046

Merged
merged 6 commits into from
Feb 14, 2024

Conversation

mpage
Copy link
Contributor

@mpage mpage commented Feb 5, 2024

These are intended to be used in places where atomics are required in free-threaded builds, but not in the default build, and we don't want to introduce the potential performance overhead of an atomic operation in the default build.

These are intended to be used in places where atomics are required in
free-threaded builds, but not in the default build, and we don't want to
introduce the potential performance overhead of an atomic operation in the
default build.
@mpage mpage marked this pull request as ready for review February 6, 2024 00:25
@mpage
Copy link
Contributor Author

mpage commented Feb 6, 2024

@corona10 @colesbury - Would you please have a look? I ended up using macros after trying experimenting with some of the changes from #114843.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the naming, I will delegate to @colesbury
2 things:

@mpage
Copy link
Contributor Author

mpage commented Feb 6, 2024

If the change does not affect to end-user side, we don't add news.d normally :)

One of the CI signals will fail without either a NEWS entry or the "skip news" label. I haven't been able to figure out how to add the "skip news" label; I suspect that I'm missing some permissions. I don't feel great about asking folks to review changes with a failing CI signal, even if it's benign, so I've been adding a NEWS entry. Do you know if it would be possible to give me (and other Cinder folks who aren't core devs) permission to add "skip news"?

@corona10
Copy link
Member

corona10 commented Feb 6, 2024

Do you know if it would be possible to give me (and other Cinder folks who aren't core devs) permission to add "skip news"?

You should be promoted to triager, at least.
But I think that we can discuss about it some moment :) cc @colesbury
Just feel free to ping me or @colesbury if you need to add the label for it.

@mpage mpage requested a review from colesbury February 13, 2024 18:37
@mpage
Copy link
Contributor Author

mpage commented Feb 13, 2024

@colesbury - Would you please take a look at this? This is the last thing we need to sort out before we can merge #113830.

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this Matt. Looks good, but a few things need to be done:

  1. Fix the merge conflict in Makefile.pre.in
  2. Add the header to pythoncore.vcxproj and pythoncore.vcxproj.filters
  3. Probably remove the NEWS file based on @corona10's comments

On (2), my strategy is usually to search for references to a nearby header (e.g., pycore_pyarena.h) in PCbuild/ and use the search results to figure out where to add the references to the new header.

@mpage mpage requested a review from a team as a code owner February 13, 2024 22:11
Makefile.pre.in Outdated Show resolved Hide resolved
@mpage
Copy link
Contributor Author

mpage commented Feb 13, 2024

@colesbury @corona10 - I think this should be good to go now. Would you please take a look?

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. @corona10 did you want to look over this as well?

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@colesbury colesbury merged commit a95b1a5 into python:main Feb 14, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants