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

Bring sleep back to WgpuAwaitable. Slowly back off on the amount of time sleeping. #655

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fyellin
Copy link
Contributor

@fyellin fyellin commented Dec 13, 2024

No description provided.

@fyellin
Copy link
Contributor Author

fyellin commented Dec 13, 2024

I've probably overengineered this.

  1. Should I be using async_sleep (defined in that file) or should I change async_sleep to use anyio? Now that we're actually sleeping, it's more important to use Event when sleeping. (Some other operations poll() might cause this callback to happen). Does everything that sniffio supports have an Event?

  2. This sleep backoff just felt kind of right to me, but I don't really have a good feel. Happy to make changes.

@fyellin fyellin marked this pull request as ready for review December 13, 2024 01:19
@fyellin fyellin requested a review from Korijn as a code owner December 13, 2024 01:19
@almarklein
Copy link
Member

Yeah, we should only rely on sniffio, because the async stuff might actually be running on rendercanvas.utils.asyncadapter, which anyio does not know about. Both sleep and Event can be obtained in the way we now get the sleep func for async_sleep (it works for asyncio/trio/rendercanvas).

@fyellin
Copy link
Contributor Author

fyellin commented Dec 14, 2024

@almarklein

The real problem is

                with anyio.move_on_after(next(sleep_generator)):
                    await self.event.wait()

This is the only way to wait on an event with a timeout intrio. (And anyio makes it work on asyncio). I can't find any documentation on rendercanvas.utils.asyncadapter so I don't know if it has something compatible. I can get rid of the events if necessary, but they do make the code more responsive.

@almarklein
Copy link
Member

Ah, I see what you mean. We could technically add support for this to rendercanvas, but the simpler solution is to not use Event here. Since we scale up the wait time, the possibly time we waited too long should only be a fraction of the total time we had to wait, so I don't see harm in that.

Could even make that explicit:

sleep_time = time_since_started / 100

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