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

uasyncio/core.py: Make current_task raise when there is no task. #11562

Closed
wants to merge 1 commit into from

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented May 19, 2023

To match CPython it should raise RuntimeError if no loop is currently active. Previously it returned the most recent task that ran.

I compared doing it this way versus setting it to None, but then we need to add the extra line to declare cur_task.

It's +60 bytes, but only +24 bytes if you don't include the error message for the RuntimeError.

Fixes #11530 CC @peterhinch

To match CPython it should raise RuntimeError if no loop is currently
active. Previously it returned the most recent task that ran.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:  +120 +0.015% standard[incl +32(data)]
      stm32:   +56 +0.014% PYBV10
        rp2:   +64 +0.020% PICO

@jimmo
Copy link
Member Author

jimmo commented May 30, 2023

I tried a few other ways to handle the cur_task variable and this was still the smallest.

@dpgeorge
Copy link
Member

The problem with this solution is performance of the asyncio loop: it's now adding another exception handler for the try-finally to the very hot path of task scheduling. And also deleting a global for each loop around the scheduler, which can be costly (a dict lookup).

I did a quick performance comparison with and without this PR and it drops performance by about 6%. The test is simply calling sleep_ms in a tight loop:

async def test(r):
    sleep_ms = asyncio.sleep_ms
    for _ in r:
        await sleep_ms(0)

@dpgeorge
Copy link
Member

See #13752 for an alternative.

@jimmo
Copy link
Member Author

jimmo commented Feb 27, 2024

Thanks @dpgeorge, yep #13752 is exactly the alternative approach I was referring to in the description

I compared doing it this way versus setting it to None, but then we need to add the extra line to declare cur_task.

Happy to go with that so will close this PR.

@jimmo jimmo closed this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uasyncio: current_task behaviour differs from CPython
2 participants