FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-128002: use per threads tasks linked list in asyncio by kumaraditya303 · Pull Request #128869 · python/cpython · GitHub

/ cpython Public

gh-128002: use per threads tasks linked list in asyncio - #128869

Merged
ambv merged 22 commits into
python:mainfrom
kumaraditya303:per-thread-tasks
Feb 6, 2025
ConversationCommits (22)ChecksFiles changed
Merged

gh-128002: use per threads tasks linked list in asyncio#128869
ambv merged 22 commits into
python:mainfrom
kumaraditya303:per-thread-tasks

Conversation

kumaraditya303 commented Jan 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Use per-thread linked list of tasks in asyncio. This design allows for lock free register/unregister of tasks of loops running concurrently in different threads. It uses the stop the world pause to traverse the list of tasks from all threads from the thread where all_tasks is called. This has no performance impact on regular builds as per benchmarks and performs a bit faster on free-threading benchmarks. pyperformance benchmarks aren't good for this because it uses just one thread so there is little lock contention, this however performs much better when multiple threads are running.

On free-threading:

Benchmark bm-20250111-linux-x86_64-python-3a570c6d58bd5ad7d7c1-3.14.0a3+-3a570c6 bm-20250112-linux-x86_64-kumaraditya303-per_thread_tasks-3.14.0a3+-cca4057
async_tree_cpu_io_mixed_tg 556 ms 536 ms: 1.04x faster
async_tree_none_tg 303 ms 294 ms: 1.03x faster
coroutines 26.2 ms 25.5 ms: 1.03x faster
async_tree_memoization_tg 397 ms 387 ms: 1.03x faster
async_tree_cpu_io_mixed 598 ms 583 ms: 1.03x faster
async_generators 498 ms 486 ms: 1.02x faster
async_tree_io 748 ms 733 ms: 1.02x faster
async_tree_io_tg 696 ms 682 ms: 1.02x faster
async_tree_memoization 442 ms 436 ms: 1.02x faster
async_tree_none 349 ms 344 ms: 1.01x faster
Geometric mean (ref) 1.02x faster

Benchmark hidden because not significant (1): asyncio_websockets

bedevere-app Bot mentioned this pull request Jan 15, 2025
Closed
9 tasks
1st1 requested review from ambv and pablogsal January 15, 2025 18:26

1st1 commented Jan 15, 2025

Copy link
Copy Markdown
Member

First, great work on this, this is legitimately a cool PR.

That said, I'm feeling really uneasy about _PyEval_StopTheWorld. Maybe instead of this approach we try spin locks + a custom mini hash map data structure? That would obviously be a lot more work, but a cleaner and ultimately better perf approach.

Also please wait for reviews from @pablogsal and @ambv. I'm curious if this would make external introspection harder.

colesbury reviewed Jan 15, 2025
View reviewed changes
Comment thread Python/pystate.c Outdated

graingert commented Jan 16, 2025

Copy link
Copy Markdown
Contributor

Does this work where an event loop is used on one thread, stopped then resumed on another thread?

This comment was marked as resolved.

Sign in to view

kumaraditya303 commented Jan 21, 2025

Copy link
Copy Markdown
Contributor Author

I have pushed some more changes:

  • Added a interpreter list of tasks which gets used when a task is alive but the thread state gets deallocated as suggested by @colesbury
  • Added thread id tracking in free-threading builds to avoid locking in the general case of loops running independently in threads without sharing tasks
  • Added some tests

TODO: benchmark it before merging

pablogsal previously requested changes Jan 22, 2025
View reviewed changes

pablogsal left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If I am not mistaken this solution seems incompatible with the asyncio introspecction workflow we are adding on #124640. Please, ensure that this change is compatible with the changes in that PR to avoid problems in the future.

bedevere-app Bot commented Jan 22, 2025

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

kumaraditya303 commented Jan 22, 2025

Copy link
Copy Markdown
Contributor Author

If I am not mistaken this solution seems incompatible with the asyncio introspecction workflow we are adding on #124640. Please, ensure that this change is compatible with the changes in that PR to avoid problems in the future.

This PR has nothing to do with asyncio introspection. As I said in other PR, the change which would affect that is moving current task to per-loop which isn't done in this PR.

ambv mentioned this pull request Jan 22, 2025
Merged
6 tasks

pablogsal commented Jan 22, 2025

Copy link
Copy Markdown
Member

Hummm, I must have misread how this affects the task management. Let me dismiss my request for changes meanwhile. Thanks for the patience with this @kumaraditya303!

kumaraditya303 commented Jan 27, 2025

Copy link
Copy Markdown
Contributor Author

@colesbury PTAL

colesbury reviewed Jan 28, 2025
View reviewed changes
Comment thread Modules/_asynciomodule.c Outdated
Comment thread Python/pystate.c Outdated
colesbury reviewed Jan 30, 2025
View reviewed changes

colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This LGTM, but would you please also get this reviewed by another asyncio expert?

ambv reviewed Feb 6, 2025
View reviewed changes
Comment thread Modules/_asynciomodule.c
ambv reviewed Feb 6, 2025
View reviewed changes
Comment thread Modules/_asynciomodule.c Outdated
ambv merged commit 0d68b14 into python:main Feb 6, 2025
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Feb 7, 2025
pythongh-128002: use per threads tasks linked list in asyncio (python…
1b70da5 
…#128869)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
kumaraditya303 deleted the per-thread-tasks branch February 7, 2025 11:59
cmaloney pushed a commit to cmaloney/cpython that referenced this pull request Feb 8, 2025
pythongh-128002: use per threads tasks linked list in asyncio (python…
d6bd7b7 
…#128869)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ambv ambv left review comments
colesbury colesbury left review comments
pablogsal pablogsal left review comments
ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
1st1 Awaiting requested review from 1st1 1st1 is a code owner
asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
willingc Awaiting requested review from willingc willingc is a code owner

Assignees

No one assigned

Labels

skip news topic-asyncio topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL