Skip to content

refactor: change GenInfo::is_running#202

Merged
KowalskiThomas merged 1 commit intoP403n1x87:mainfrom
KowalskiThomas:kowalski/refactor-change-geninfo-is_running
Dec 4, 2025
Merged

refactor: change GenInfo::is_running#202
KowalskiThomas merged 1 commit intoP403n1x87:mainfrom
KowalskiThomas:kowalski/refactor-change-geninfo-is_running

Conversation

@KowalskiThomas
Copy link
Collaborator

What does this PR do?

This PR changes the way GenInfo::is_running works. Previously, it would indicate whether the current coroutine was on CPU; now, it indicates whether the current coroutine or the coroutine it (recursively) awaits is on CPU.
Making that change also allows us to do less work when we check whether the current coroutine is on CPU or not. Because a Coroutine / Generator / GenInfo can only be running if it is not awaiting another Generator, we do not need to compute is_running if we have await != nullptr (and we take await->is_running for the value in that case).

Making that change makes it easier to check whether a Task is currently on-CPU; and allows to do less work when we decide how to unwind asyncio Tasks (cf the changes in TaskInfo which doesn't need the is_on_cpu method that iterates on the await chain anymore).

Note that I checked whether GenInfo::is_running was used in any other way than the one I describe and simplify; it is not, so I do think this change is safe to make as-is.

Note This PR makes sense on its own, but it is in the context of making #198 simpler.

@KowalskiThomas KowalskiThomas changed the title refactor: change GenInfo::is_running refactor: change GenInfo::is_running Dec 4, 2025
@KowalskiThomas KowalskiThomas marked this pull request as ready for review December 4, 2025 09:39
Copy link
Collaborator

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LGTM
Ok to iterate on this version as it will make the timeline easier to read.
Though we might want to differentiate these on_cpu cases.

@KowalskiThomas
Copy link
Collaborator Author

For posterity

Though we might want to differentiate these on_cpu cases.

Definitely agree – I initially meant to have two fields, is_running and waiter_is_running or something like that. However, since we would currently only use waiter_is_running, I decided to go for updating is_running – we can always change that in the future if we need to, it's a pretty small and easy change!

@KowalskiThomas KowalskiThomas merged commit 19938ea into P403n1x87:main Dec 4, 2025
60 of 66 checks passed
KowalskiThomas added a commit to DataDog/dd-trace-py that referenced this pull request Dec 5, 2025
## Description

This change replicates P403n1x87/echion#202.

This PR changes the way `GenInfo::is_running` works. Previously, it
would indicate whether the _current_ coroutine was on CPU; now, it
indicates whether the current coroutine _or the coroutine it
(recursively) awaits_ is on CPU.
Making that change also allows us to do less work when we check whether
the current coroutine is on CPU or not. Because a Coroutine / Generator
/ `GenInfo` can only be running if it is not awaiting another Generator,
we do not need to compute `is_running` if we have `await != nullptr`
(and we take `await->is_running` for the value in that case).

Making that change makes it easier to check whether a Task is currently
on-CPU; and allows to do less work when we decide how to unwind
`asyncio` Tasks (cf the changes in `TaskInfo` which doesn't need the
`is_on_cpu` method that iterates on the `await` chain anymore).

Note that I checked whether `GenInfo::is_running` was used in any other
way than the one I describe and simplify; it is not, so I do think this
change is safe to make as-is.

**Note** This PR makes sense on its own, but it is in the context of
making P403n1x87/echion#198 simpler.

**Note** This doesn't need a changelog entry as it makes no difference
to the user, it's purely a refactor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants