GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-115041: Add wrappers that are atomic only in free-threaded builds by mpage · Pull Request #115046 · python/cpython

Conversation

@mpage

@mpage mpage commented Feb 5, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

  • Issue: Atomic operations for only free-threaded builds #115041

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mpage
Add wrappers that are atomic only in free-threaded builds
2b3690d 
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.
@bedevere-app bedevere-app Bot mentioned this pull request Feb 5, 2024
Atomic operations for only free-threaded builds #115041
Closed
@blurb-it
📜🤖 Added by blurb_it.
4cd4621
@mpage
mpage marked this pull request as ready for review February 6, 2024 00:25
@bedevere-app bedevere-app Bot added the awaiting review label Feb 6, 2024
@mpage

mpage commented Feb 6, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 added the skip news label Feb 6, 2024
corona10
corona10 reviewed Feb 6, 2024
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

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.

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

  • If the change does not affect to end-user side, we don't add news.d normally :)
  • You may need to update pythoncore.vcxproj and pythoncore.vcxproj.filters for newly added headers.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mpage

mpage commented Feb 6, 2024

Copy link
Copy Markdown
Contributor Author

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"?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Feb 6, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mpage
mpage requested a review from colesbury February 13, 2024 18:37
@mpage

mpage commented Feb 13, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury
colesbury reviewed Feb 13, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mpage reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
mpage added 3 commits February 13, 2024 13:45
@mpage
Remove NEWS entry
1b977a5
@mpage
Merge branch 'main' into pythongh-115041-ft-only-atomics
9978056
@mpage
Add header to vcxproj files
e78e018
@mpage
mpage requested a review from a team as a code owner February 13, 2024 22:11
colesbury
colesbury reviewed Feb 13, 2024
View reviewed changes
Comment thread Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.
@mpage
Remove stray newline
b44e1ab
@mpage
mpage force-pushed the gh-115041-ft-only-atomics branch from bfe4934 to b44e1ab Compare February 13, 2024 22:32
@mpage

mpage commented Feb 13, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury
colesbury approved these changes Feb 13, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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 LGTM. @corona10 did you want to look over this as well?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 13, 2024
corona10
corona10 approved these changes Feb 14, 2024
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

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.

LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury
colesbury merged commit a95b1a5 into python:main Feb 14, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury approved these changes
@corona10 corona10 corona10 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@mpage @corona10 @colesbury
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.