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

Use setTimeout() trick instead of Window.requestIdleCallback() #3044

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

daxpedda
Copy link
Member

This reverts #2880 and instead uses a trick to prevent setTimeout() to throttle to 4ms.
On Chrome this should use the new Prioritized Task Scheduling API instead, which I will implement as a follow up.

Using Window.requestIdleCallback() should still remain an option for it's tradeoffs, which can be re-implemented after #3042.

Cc @GloopShlugger.

@GloopShlugger
Copy link

GloopShlugger commented Aug 24, 2023

This method seems to be very unstable, the frametimes vary a lot on both firefox and chrome for me, it does however fix the issue. I'll create a small minimal reproducible example in hopes of narrowing it down, I think idleCallback is the lesser of the two evils here and should be preferred. Maybe something like a rust version of the javascript noop requestAnimationFrame could be ductaped onto it

@GloopShlugger
Copy link

GloopShlugger commented Aug 25, 2023

Update: It is not a bug in winit 10000% confirmed, requestIdleCallback is fine
i cannot reproduce it locally :)
i tried:

  • just wasm in an iframe -> works perfectly
  • wasm from another server (to emulate what itch.io is doing) -> works perfectly
  • put it on itch.io -> doesn't work

the actual workload in the loop seems to make no difference

Seeing as this is strictly worse than requestIdleCallback and more hacky, I'd suggest closing it in favor of the other version.

It feels out of scope for winit to try and solve as it's likely a browser implementation detail
I think the best that could be done here is to advise users to use set_wait if they plan to host on itch and want it to run every frame but that feels very specific

@daxpedda
Copy link
Member Author

daxpedda commented Aug 25, 2023

Could you specify what exactly doesn't work?

This patch doesn't actually touch rAF (Window::request_redraw()/RedrawRequested) but it makes sure that ControlFlow::Poll isn't throttled. Could you confirm that you when you are using ControlFlow::Poll the Event::NewEvents(StartCause::Poll) event doesn't get throttled?

Update: It is not a bug in winit 10000% confirmed, requestIdleCallback is fine
i cannot reproduce it locally :)

I wouldn't dismiss the possibility out of hand, we don't understand their throttling algorithm, so it might be that itch.io is causing throttling somehow that we don't know how to reproduce.

Thank you so much for looking into it though.

@GloopShlugger
Copy link

GloopShlugger commented Aug 25, 2023

This patch doesn't actually touch rAF (Window::request_redraw()/RedrawRequested) but it makes sure that ControlFlow::Poll isn't throttled. Could you confirm that you when you are using ControlFlow::Poll the Event::NewEvents(StartCause::Poll) event doesn't get throttled?

newEvents using this PR does indeed run as fast as it can with something about 0.2ms on firefox and ~2ms on chrome inbetween, it does however cause redrawrequested to skip about 5ish frames so it runs at 12fps mostly. After more testing it seems that's only an issue on firefox because of course it is

whereas with IdleCallback you get one event in sync with the redraw and that gets called as you'd expect at about 60fps.

Could you specify what exactly doesn't work?

I was refering to the loop-pausing-bug appearing, only appears on itch

I preemptively claimed this method as worse before checking deeper
It solves the loop-pause bug to be clear, but i thought i felt higher framerate variabilty on chrome when testing which seems to have been a misjudgement

do you think there's a way to keep this behaviour on chrome and increase the timeout on firefox to something comparable?

I tested in the place where the timeout gets requested when duration is None, to just set it to 2ms and that fixes the performance issue on firefox and keeps working on chrome so it seems to be just a busyness issue, im not sure how it degrades when redrawrequested takes longer or some other event handler takes longer (or if that even matters)

that might be the """"fix"""" unless you see glaring issues, other than it turning Poll into Wait with a nice dress
note: the firefox degradation also appears with this pr using set_wait_timeout(0) which makes sense

@daxpedda
Copy link
Member Author

daxpedda commented Aug 25, 2023

newEvents using this PR does indeed run as fast as it can with something about 0.2ms on firefox and ~2ms on chrome inbetween, it does however cause redrawrequested to skip about 5ish frames so it runs at 12fps mostly. After more testing it seems that's only an issue on firefox because of course it is

! Actually after some testing I reproduced this. Will debug it and see if I can fix it.

do you think there's a way to keep this behaviour on chrome and increase the timeout on firefox to something comparable?

I tested in the place where the timeout gets requested when duration is None, to just set it to 2ms and that fixes the performance issue on firefox and keeps working on chrome so it seems to be just a busyness issue, im not sure how it degrades when redrawrequested takes longer or some other event handler takes longer (or if that even matters)

I think the issue in general is just that Firefox is throttled badly because setTimeout() is called a LOT of times. On Chrome I will switch to the Prioritized Task Scheduling API, that should pretty much behave the same but a bit better. On Firefox there is currently no equal solution but I will see what I can do to fix it, what's going on right now is definitely not desirable.

In general though I'm not entirely sure what the use-case for ControlFlow::Poll is, it will just consume 100% CPU, I'm not sure who wants that, especially on Web. Putting a ControlFlow::WaitUntil in-between is always good, basically some form of tick-rate.

In any case, will report back.

@daxpedda
Copy link
Member Author

Okay, so this has nothing to do with Winit indeed. Firefox is just not very good with handling this kind of performance pressure.

I would recommend you to not use ControlFlow::Poll without some throttling from your side, on both Chrome and Firefox, it will simply consume 100% CPU, I doubt that is want you want your users to experience.

After I implement #3042 I can expose Window.requestIdleCallback() again if you still prefer that.

@daxpedda
Copy link
Member Author

Chrome should work even better now.
I plan to merge this now as is, but please feel free to continue the discussion here if there are any more issues or if you have any other suggestions.

@GloopShlugger
Copy link

I think that's reasonable, one concern i have is how it might affect the ecosystem as polling is the default
and i think bevy uses poll aswell
originally it sounded like poll was used / is supposed to be used to "just restart the loop when drawing is done"
but with the seperation that might just not make sense anymore

@daxpedda
Copy link
Member Author

daxpedda commented Aug 25, 2023

I don't have enough knowledge about how ControlFlow::Poll is supposed to be used, I basically never used it outside of testing.
@kchibisov do you have any input on this?

@GloopShlugger
Copy link

tested and wow yeah chrome now has the same delay as firefox
great work thank you for the help

@daxpedda daxpedda merged commit 48abf52 into rust-windowing:master Aug 25, 2023
55 checks passed
@daxpedda daxpedda mentioned this pull request Aug 10, 2023
25 tasks
kchibisov pushed a commit to kchibisov/winit that referenced this pull request Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants