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.

Sign up for GitHub

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

Jump to bottom

gh-120974: Make _asyncio._enter_task atomic in the free-threaded build #122138

Merged
merged 1 commit into from
Jul 23, 2024
Merged

gh-120974: Make _asyncio._enter_task atomic in the free-threaded build #122138

merged 1 commit into from
Jul 23, 2024

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Jul 22, 2024 •
edited by bedevere-app bot
Loading

Use PyDict_SetDefaultRef to set the current task in a single operation under the dictionary's lock.

@colesbury
pythongh-120974: Make _asyncio._enter_task atomic in the free-threade…
2f638e9 
…d build

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
@bedevere-app bedevere-app bot mentioned this pull request Jul 22, 2024
Closed
@colesbury
Copy link
Contributor Author

Still need to address leave_task and swap_current_task, but this was the easiest function to start with.

The goal with this PR is thread-safety in the free-threaded build, but it also seems to slightly improve performance of enter_task.

Here's a little microbenchmark I'm using: https://gist.github.com/colesbury/ece2f1a5cbc4c50f86c8634744a9badc.
BEFORE: ~102 ns per enter_task and leave_task call pair
AFTER: ~92 ns per call pair

@colesbury colesbury mentioned this pull request Jul 22, 2024
Merged
kumaraditya303
kumaraditya303 approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM

@kumaraditya303 kumaraditya303 merged commit 47847aa into python:main Jul 23, 2024
40 checks passed
@miss-islington-app
Copy link

Thanks @colesbury for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 23, 2024
@colesbury @miss-islington
pythongh-120974: Make _asyncio._enter_task atomic in the free-threade…
8faab2e 
…d build (pythonGH-122138)

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
(cherry picked from commit 47847aa)

Co-authored-by: Sam Gross <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jul 23, 2024

GH-122152 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 Jul 23, 2024
kumaraditya303 pushed a commit that referenced this pull request Jul 23, 2024
@miss-islington @colesbury
[3.13] gh-120974: Make _asyncio._enter_task atomic in the free-thread…
48154e7 
…ed build (GH-122138) (#122152)

gh-120974: Make _asyncio._enter_task atomic in the free-threaded build (GH-122138)

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
(cherry picked from commit 47847aa)

Co-authored-by: Sam Gross <[email protected]>
@colesbury colesbury deleted the gh-120974-asyncio-enter-task branch July 23, 2024 14:36
nohlson pushed a commit to nohlson/cpython that referenced this pull request Jul 24, 2024
@colesbury @nohlson
pythongh-120974: Make _asyncio._enter_task atomic in the free-threade…
26af5f7 
…d build (python#122138)

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
nohlson pushed a commit to nohlson/cpython that referenced this pull request Jul 24, 2024
@colesbury @nohlson
pythongh-120974: Make _asyncio._enter_task atomic in the free-threade…
6b74fbe 
…d build (python#122138)

Use `PyDict_SetDefaultRef` to set the current task in a single operation
under the dictionary's lock.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

@willingc willingc Awaiting requested review from willingc willingc is a code owner

Assignees
No one assigned
Labels
skip news topic-free-threading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@colesbury @kumaraditya303