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.

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

Double linked list implementation for asyncio tasks #107803

Closed
kumaraditya303 opened this issue Aug 9, 2023 · 4 comments
Closed

Double linked list implementation for asyncio tasks #107803

kumaraditya303 opened this issue Aug 9, 2023 · 4 comments
Assignees
Labels
3.14 new features, bugs and security fixes performance Performance or resource usage topic-asyncio

Comments

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Aug 9, 2023

Currently asyncio tasks are stored in a Weakset, this is inefficient and in some cases causes bugs because of thread safety (#80788). In terms of memory usage it requires maintaining a full set and their corresponding weakref callback to cleanup objects when deallocated and finalized by the gc. In applications where tasks are created at fast pace this becomes a bottle neck, to mitigate this now asyncio tasks will now be stored in a global double linked of tasks for cases where Task is a subclass of _asyncio.Task in other cases we still rely on the weakset. This reduces the work done by the gc speedups the execution and reduces memory usage. In some of my own benchmarks I have seen 15- 20% improvement and pyperformance benchmarks reflect roughly the same.

https://github.com/faster-cpython/benchmarking-public/blob/main/results/bm-20230805-3.13.0a0-1d32835/bm-20230805-linux-x86_64-kumaraditya303-linked_list-3.13.0a0-1d32835-vs-base.md

Updated: https://github.com/faster-cpython/benchmarking-public/tree/main/results/bm-20240622-3.14.0a0-4717aaa#vs-base

Linked PRs

@kumaraditya303 kumaraditya303 added performance Performance or resource usage topic-asyncio 3.13 bugs and security fixes labels Aug 9, 2023
@kumaraditya303 kumaraditya303 self-assigned this Aug 9, 2023
willingc pushed a commit that referenced this issue Jun 22, 2024
…7804)

* linked list

* add tail optmiization to linked list

* wip

* wip

* wip

* more fixes

* finally it works

* add tests

* remove weakreflist

* add some comments

* reduce code duplication in _asynciomodule.c

* address some review comments

* add invariants about the state of the linked list

* add better explanation

* clinic regen

* reorder branches for better branch prediction

* Update Modules/_asynciomodule.c

* Apply suggestions from code review

Co-authored-by: Itamar Oren <[email protected]>

* fix capturing of eager tasks

* add comment to task finalization

* fix tests and couple c implmentation to c task

improved linked-list logic and more comments

* fix test

---------

Co-authored-by: Itamar Oren <[email protected]>
@kumaraditya303 kumaraditya303 added 3.14 new features, bugs and security fixes and removed 3.13 bugs and security fixes labels Jun 23, 2024
@colesbury
Copy link
Contributor

I think there are some thread-safety issues with the linked list implementation:

  • It's not going to be thread-safe in the free-threaded build. We can probably add a lock or a critical section.
  • Even with the GIL, the linked list of tasks may be modified while it's being looped over because the GIL can be released in add_one_task.

@willingc
Copy link
Contributor

Thanks @colesbury for flagging this.

@kumaraditya303
Copy link
Contributor Author

I am closing this, will track free threaded issues in #120974

@colesbury
Copy link
Contributor

@kumaraditya303, can you fix the thread-safety/reentrancy issue that affects the default build (with the GIL)? head can be deallocated while it's being used. It needs to be protected by a Py_INCREF(). This is not specific to the free-threading build.

TaskObj *tail = &state->asyncio_tasks.tail;
while (head != tail)
{
if (add_one_task(state, tasks, (PyObject *)head, loop) < 0) {
Py_DECREF(tasks);
Py_DECREF(loop);
return NULL;
}
head = head->next;
assert(head != NULL);
}

mrahtz pushed a commit to mrahtz/cpython that referenced this issue Jun 30, 2024
…ythonGH-107804)

* linked list

* add tail optmiization to linked list

* wip

* wip

* wip

* more fixes

* finally it works

* add tests

* remove weakreflist

* add some comments

* reduce code duplication in _asynciomodule.c

* address some review comments

* add invariants about the state of the linked list

* add better explanation

* clinic regen

* reorder branches for better branch prediction

* Update Modules/_asynciomodule.c

* Apply suggestions from code review

Co-authored-by: Itamar Oren <[email protected]>

* fix capturing of eager tasks

* add comment to task finalization

* fix tests and couple c implmentation to c task

improved linked-list logic and more comments

* fix test

---------

Co-authored-by: Itamar Oren <[email protected]>
mrahtz pushed a commit to mrahtz/cpython that referenced this issue Jun 30, 2024
mrahtz pushed a commit to mrahtz/cpython that referenced this issue Jun 30, 2024
facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this issue Jul 3, 2024
Summary:
upstream issue: python/cpython#107803

backported commits:
- python/cpython@4717aaa
- python/cpython@8223544

Differential Revision: D59280319

fbshipit-source-id: 9fb14b7f5b6662ff5093ed27c56841b8de8c5a2c
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
…ythonGH-107804)

* linked list

* add tail optmiization to linked list

* wip

* wip

* wip

* more fixes

* finally it works

* add tests

* remove weakreflist

* add some comments

* reduce code duplication in _asynciomodule.c

* address some review comments

* add invariants about the state of the linked list

* add better explanation

* clinic regen

* reorder branches for better branch prediction

* Update Modules/_asynciomodule.c

* Apply suggestions from code review

Co-authored-by: Itamar Oren <[email protected]>

* fix capturing of eager tasks

* add comment to task finalization

* fix tests and couple c implmentation to c task

improved linked-list logic and more comments

* fix test

---------

Co-authored-by: Itamar Oren <[email protected]>
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
…ythonGH-107804)

* linked list

* add tail optmiization to linked list

* wip

* wip

* wip

* more fixes

* finally it works

* add tests

* remove weakreflist

* add some comments

* reduce code duplication in _asynciomodule.c

* address some review comments

* add invariants about the state of the linked list

* add better explanation

* clinic regen

* reorder branches for better branch prediction

* Update Modules/_asynciomodule.c

* Apply suggestions from code review

Co-authored-by: Itamar Oren <[email protected]>

* fix capturing of eager tasks

* add comment to task finalization

* fix tests and couple c implmentation to c task

improved linked-list logic and more comments

* fix test

---------

Co-authored-by: Itamar Oren <[email protected]>
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.14 new features, bugs and security fixes performance Performance or resource usage topic-asyncio
Projects
Status: Done
Development

No branches or pull requests

3 participants