Skip to content

gh-129533: Update PyGC_Enable/Disable/IsEnabled to use atomic operation #129563

Merged
corona10 merged 7 commits intopython:mainfrom
corona10:gh-129533
Feb 6, 2025
Merged

gh-129533: Update PyGC_Enable/Disable/IsEnabled to use atomic operation #129563
corona10 merged 7 commits intopython:mainfrom
corona10:gh-129533

Conversation

@corona10
Copy link
Copy Markdown
Member

@corona10 corona10 commented Feb 2, 2025 •
edited by bedevere-app bot
Loading

@corona10 corona10 requested a review from colesbury February 2, 2025 04:13
@bedevere-app bedevere-app bot mentioned this pull request Feb 2, 2025
Closed
@corona10 corona10 requested a review from vstinner February 2, 2025 04:13
@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Feb 2, 2025

TSAN is passed at my local machine:

import concurrent.futures
import functools
import gc
import threading

num_threads = 10

def closure(b):
  b.wait()
  for _ in range(100):
      gc.disable()
      gc.enable()

with concurrent.futures.ThreadPoolExecutor(max_workers=num_threads) as executor:
  for i in range(100):
    b = threading.Barrier(num_threads)
    for _ in range(num_threads):
      executor.submit(functools.partial(closure, b))

corona10
corona10 commented Feb 2, 2025
View reviewed changes
Python/gc_free_threading.c Outdated
int old_state = gcstate->enabled;
gcstate->enabled = 0;
int old_state = _Py_atomic_load_int(&(gcstate->enabled));
while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 0));
Copy link
Copy Markdown
Member Author

@corona10 corona10 Feb 2, 2025

Choose a reason for hiding this comment

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

We don't need to update old_state explicitly since __atomic_compare_exchange_n automatically handle.
https://gcc.gnu.org/onlinedocs/gcc-8.4.0/gcc/_005f_005fatomic-Builtins.html

@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Feb 2, 2025

cc @hawkinsp

@corona10 corona10 requested a review from pablogsal February 2, 2025 04:48
mpage
mpage reviewed Feb 2, 2025
View reviewed changes
@corona10 @mpage
Apply suggestions from code review
8a41f07 
Co-authored-by: mpage <mpage@cs.stanford.edu>
@corona10 corona10 requested a review from mpage February 2, 2025 07:13
@corona10 corona10 changed the title gh-129533: Update PyGC_Enable/Disable/IsEnabled to use CAS operation gh-129533: Update PyGC_Enable/Disable/IsEnabled to use atomic operation Feb 2, 2025
vstinner
vstinner approved these changes Feb 2, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

Python/gc_free_threading.c Outdated
GCState *gcstate = get_gc_state();
int old_state = gcstate->enabled;
gcstate->enabled = 1;
int old_state = _Py_atomic_exchange_int(&gcstate->enabled, 1);
Copy link
Copy Markdown
Member

@vstinner vstinner Feb 2, 2025

Choose a reason for hiding this comment

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

You can remove the old_state variable. Same remark in PyGC_Disable() and PyGC_IsEnabled().

@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Feb 2, 2025

Waiting @pablogsal before merging this PR :)

@corona10 corona10 merged commit b184abf into python:main Feb 6, 2025
@miss-islington-app
Copy link
Copy Markdown

miss-islington-app bot commented Feb 6, 2025

Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link
Copy Markdown

miss-islington-app bot commented Feb 6, 2025

Sorry, @corona10, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker b184abf074c0e1f379a238f07da5616460f36b93 3.13

corona10 added a commit to corona10/cpython that referenced this pull request Feb 6, 2025
@corona10
pythongh-129533: Update PyGC_Enable/Disable/IsEnabled to use atomic …
aea5951 
…operation (pythongh-129563)

(cherry picked from commit b184abf)
@corona10 corona10 mentioned this pull request Feb 6, 2025
Merged
@bedevere-app
Copy link
Copy Markdown

bedevere-app bot commented Feb 6, 2025

GH-129756 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Feb 6, 2025
corona10 added a commit that referenced this pull request Feb 6, 2025
@corona10
[3.13] gh-129533: Update PyGC_Enable/Disable/IsEnabled to use atomic…
f7af8bc 
… operat… (gh-129756)

gh-129533: Update PyGC_Enable/Disable/IsEnabled to use atomic operation  (gh-129563)

(cherry picked from commit b184abf)
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Feb 7, 2025
@corona10 @srinivasreddy
pythongh-129533: Update PyGC_Enable/Disable/IsEnabled to use atomic …
9e29b4f 
…operation (pythongh-129563)
cmaloney pushed a commit to cmaloney/cpython that referenced this pull request Feb 8, 2025
@corona10 @cmaloney
pythongh-129533: Update PyGC_Enable/Disable/IsEnabled to use atomic …
8df34e9 
…operation (pythongh-129563)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@pablogsal pablogsal Awaiting requested review from pablogsal

@mpage mpage Awaiting requested review from mpage

Assignees

@corona10 corona10

Labels

topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@corona10 @vstinner @mpage @kumaraditya303