Skip to content

gh-128588: gh-128550: remove eager tasks optimization that missed and introduced incorrect cancellations #129063

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
Merged
merged 7 commits into from
Jan 20, 2025
Merged

gh-128588: gh-128550: remove eager tasks optimization that missed and introduced incorrect cancellations #129063

merged 7 commits into from
Jan 20, 2025

Conversation

graingert
Copy link
Contributor

@graingert graingert commented Jan 20, 2025 •
edited
Loading

@graingert
pythongh-128588: pythongh-128550: remove eager tasks optimization tha…
9fce599 
…t missed and introduced incorrect cancellations
@bedevere-app bedevere-app bot mentioned this pull request Jan 20, 2025
Closed
@graingert graingert mentioned this pull request Jan 20, 2025
Closed
@graingert graingert marked this pull request as ready for review January 20, 2025 13:23
@kumaraditya303 kumaraditya303 enabled auto-merge (squash) January 20, 2025 16:45
@kumaraditya303 kumaraditya303 added type-bug An unexpected behavior, bug, or error topic-asyncio needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jan 20, 2025
@kumaraditya303 kumaraditya303 merged commit ed6934e into python:main Jan 20, 2025
50 checks passed
@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 20, 2025
@graingert @kumaraditya303 @miss-islington
pythongh-128588: pythongh-128550: remove eager tasks optimization tha…
70fb2d0 
…t missed and introduced incorrect cancellations (pythonGH-129063)

(cherry picked from commit ed6934e)

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
@miss-islington-app
Copy link

Sorry, @graingert and @kumaraditya303, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker ed6934e71e55d398df8263f4697f58e4a3815f69 3.12

@bedevere-app
Copy link

bedevere-app bot commented Jan 20, 2025

GH-129089 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 Jan 20, 2025
@graingert graingert deleted the fix-eager-tasks-early-callback branch January 20, 2025 17:36
kumaraditya303 added a commit that referenced this pull request Jan 20, 2025
@miss-islington @graingert @kumaraditya303
[3.13] gh-128588: gh-128550: remove eager tasks optimization that mis…
a1c48a7 
…sed and introduced incorrect cancellations (GH-129063) (#129089)

gh-128588: gh-128550: remove eager tasks optimization that missed and introduced incorrect cancellations (GH-129063)
(cherry picked from commit ed6934e)

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
graingert added a commit to graingert/cpython that referenced this pull request Jan 20, 2025
@graingert @kumaraditya303
pythongh-128588: pythongh-128550: remove eager tasks optimization tha…
59aff22 
…t missed and introduced incorrect cancellations (python#129063)

Co-authored-by: Kumar Aditya <kumaraditya@python.org>
@itamaro
Copy link
Contributor

itamaro commented Jan 20, 2025

What's the impact of removing this optimization on the async_tree pyperformance benchmarks?

@kumaraditya303
Copy link
Contributor

What's the impact of removing this optimization on the async_tree pyperformance benchmarks?What's the impact of removing this optimization on the async_tree pyperformance benchmarks?

I haven't benchmarked yet, I'll do it but I expect an insignificant impact.

kumaraditya303 added a commit that referenced this pull request Jan 21, 2025
@graingert @kumaraditya303
[3.12] gh-128588: fix refcycles in eager task creation and remove eag…
bc21454 
…er tasks optimization that missed and introduced incorrect cancellations (#129063) (#128586)

Co-authored-by: Kumar Aditya <kumaraditya@python.org>
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
@graingert @kumaraditya303 @srinivasreddy
pythongh-128588: pythongh-128550: remove eager tasks optimization tha…
c765847 
…t missed and introduced incorrect cancellations (python#129063)

Co-authored-by: Kumar Aditya <kumaraditya@python.org>
@itamaro
Copy link
Contributor

itamaro commented Feb 2, 2025

I haven't benchmarked yet, I'll do it but I expect an insignificant impact.

any update on benchmarking results @kumaraditya303 ?

benchmarking of the original PR showed up to 4x speedup on some of the async benchmarks. anything changed that you expect removing the optimization would have insignificant impact?

@kumaraditya303
Copy link
Contributor

any update on benchmarking results kumaraditya303 ?
benchmarking of #104140 showed up to 4x speedup on some of the async benchmarks. anything changed that you expect removing the optimization would have insignificant impact?

I had asked Michael to benchmark this last week but I haven't heard back from them, I'll ping them again but meanwhile I compared the rough number by comparing benchmarks with and without this patch and I don't see any large slowdown on the benchmarks:

With this PR:

image

Without this PR:
image

@kumaraditya303
Copy link
Contributor

The results are now at https://github.com/faster-cpython/benchmarking-public/blob/main/results/bm-20250120-3.14.0a4%2B-ed6934e/bm-20250120-linux-x86_64-python-ed6934e71e55d398df82-3.14.0a4%2B-ed6934e-vs-base.md#benchmarks-with-tag-asyncio and there's no slowdown.

@itamaro
Copy link
Contributor

itamaro commented Feb 9, 2025

Hmm, I think I see what's going on here.. Looking more closely at the original PR, it appears the benchmarking used a patched version of pyperformance to add an "eager" flavor to the async tree benchmarks. Since these flavors don't exist in the released version of pyperformance, we don't see them in the benchmark runs...

@hugovk hugovk removed the needs backport to 3.12 only security fixes label Feb 26, 2025
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

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

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

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

Assignees

@kumaraditya303 kumaraditya303

Labels
topic-asyncio type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@graingert @itamaro @kumaraditya303 @hugovk