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

Web backend sends RedrawRequested too often #1305

Open
ryanisaacg opened this issue Dec 3, 2019 · 13 comments
Open

Web backend sends RedrawRequested too often #1305

ryanisaacg opened this issue Dec 3, 2019 · 13 comments

Comments

@ryanisaacg
Copy link
Member

@ryanisaacg ryanisaacg commented Dec 3, 2019

An expected user pattern with the new API is to request redraw after every EventsCleared.

On web, the event handlers are run instantly as events come in, which means there can be many more EventsCleared than the user expects.

I can think of two ways to address this:

  • Don't run the event handlers as often, and buffer up events with a minimum time between processing them. This adds latency where it isn't necessarily needed, so it may not be ideal.
  • Only run RedrawRequested events on a timer determined by the browser requestAnimationFrame API, which does slightly break the API contract.

I'm not sure which of these mitigations is better, so I would welcome discussion.

@Osspial

This comment has been minimized.

Copy link
Member

@Osspial Osspial commented Dec 3, 2019

Why is this a problem? This behavior is also exhibited on Windows since there's pretty much no overhead between the platform event dispatcher and the Winit event closure, but I haven't found it to be an issue because real applications take long enough to process events (due to vsync, rendering, world updates, etc.) that events naturally get buffered up. Even if downstream users have unusually lightweight applications that can reach extreme framerates, that shouldn't pose any functional issues in their applications.

@ryanisaacg

This comment has been minimized.

Copy link
Member Author

@ryanisaacg ryanisaacg commented Dec 5, 2019

My understanding from a few user reports is that the redraws 'build up' over time, consistently producing more work than is handled.

@goddessfreya

This comment has been minimized.

Copy link
Member

@goddessfreya goddessfreya commented Dec 5, 2019

Won't redraw buildup be resolved by redraw-requested-v2?

@ryanisaacg

This comment has been minimized.

Copy link
Member Author

@ryanisaacg ryanisaacg commented Dec 9, 2019

It might be; once it lands on master, we can see if that fixes the problem.

@Osspial

This comment has been minimized.

Copy link
Member

@Osspial Osspial commented Dec 21, 2019

Do web platforms automatically aggregate duplicate RedrawRequested events into a single event?

@ryanisaacg

This comment has been minimized.

Copy link
Member Author

@ryanisaacg ryanisaacg commented Dec 28, 2019

Yes, if you request redraw multiple times before another RedrawRequested it should only produce one event

@tangmi

This comment has been minimized.

Copy link
Contributor

@tangmi tangmi commented Jan 3, 2020

Edit 2: Ignore everything quoted below, I misunderstood the situation!


@ryanisaacg I agree that RedrawRequested should only fire when requestAnimationFrame runs, but not necessarily every frame (so that non-game applications that don't change often can save battery life/cpu cycles). What do you think of a flag that producers of RedrawRequested can set/check to kick off a single requestAnimationFrame that is cleared when the requestAnimationFrame callback finishes? This would effectively debounce RedrawRequested events and let the browser service the "request" when it best thinks it should. A downside of this is that the visuals could always be a frame behind (but I think that might always be the case if visuals are always handled in requestAnimationFrame's callback...?)

Edit: after writing this, I realize this might be a much more convoluted way of describing what was discussed already/exists in code... In either case, I believe it's important to deduplicate events on web, if possible. I recall in the previous web (emscripten) backend, the main js event loop could be saturated with (winit) events, causing slowdowns just from winit/emscripten overhead.

@OvermindDL1

This comment has been minimized.

Copy link

@OvermindDL1 OvermindDL1 commented Jan 10, 2020

(but I think that might always be the case if visuals are always handled in requestAnimationFrame's callback...?)

From my understanding, requestAnimationFrame callback is called immediately before the browser renders a new frame, so there shouldn't be any delay?

@tangmi

This comment has been minimized.

Copy link
Contributor

@tangmi tangmi commented Jan 10, 2020

@ryanisaacg what do you think of using requestAnimationFrame as a timer of sorts for aggregating events into individual "frames"? Like, when any event happens, call requestAnimationFrame (and set a flag) and fire a NewEvents. Continue processing events and when the callback fires, trigger the MainEventsCleared event and then RedrawRequested (if the app has called Window::request_redraw) and clear the flag.

This way, MainEventsCleared/RedrawRequested only fires once per frame, but if no events are coming in it is not fired (nor is requestAnimationFrame called).

Edit: for posterity, I think the relevant code is here:

// Handle starting a new batch of events
//
// The user is informed via Event::NewEvents that there is a batch of events to process
// However, there is only one of these per batch of events
self.handle_event(Event::NewEvents(start_cause), &mut control);
if !event_is_start {
self.handle_event(event, &mut control);
}
self.handle_event(Event::MainEventsCleared, &mut control);
// Collect all of the redraw events to avoid double-locking the RefCell
let redraw_events: Vec<WindowId> = self.0.redraw_pending.borrow_mut().drain().collect();
for window_id in redraw_events {
self.handle_event(Event::RedrawRequested(window_id), &mut control);
}
self.handle_event(Event::RedrawEventsCleared, &mut control);

@OvermindDL1

This comment has been minimized.

Copy link

@OvermindDL1 OvermindDL1 commented Jan 10, 2020

Something I do recall having an issue with with requestAnimationFrame is that if updating of the DOM or drawing anything or so happens outside of it's callback, the browser is free to interleave things, so you can get tearing and so forth. Just for note.

@tangmi

This comment has been minimized.

Copy link
Contributor

@tangmi tangmi commented Jan 11, 2020

@OvermindDL1 I don't think that should be an issue for winit, but do you have docs describing that behavior? I thought that dom updates were always atomic (I guess, perks of being on the web vs e.g. wincomp) and that one of the reasons requestAnimationFrame exists is to force vsync.

@ryanisaacg

This comment has been minimized.

Copy link
Member Author

@ryanisaacg ryanisaacg commented Jan 13, 2020

I think we haven't seen new reports of this problem, and the events are being de-duplicated correctly. Aligning to requestAnimationFrame still might be a good idea, at least as a platform-specific setting.

@OvermindDL1

This comment has been minimized.

Copy link

@OvermindDL1 OvermindDL1 commented Jan 13, 2020

@tangmi DOM updates themselves are atomic, however the rendered state of them is not, that happens asyncronously to the DOM updates in the common browsers.

https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Performance_best_practices_for_Firefox_fe_engineers

Note that requestAnimationFrame() lets you queue up JavaScript to run right before the style flush occurs. This allows you to put all of your DOM writes (most importantly, anything that could change the size or position of things in the DOM) just before the style and layout steps of the pipeline, combining all the style and layout calculations into a single batch so it all happens once, in a single frame tick, instead of across multiple frames.

https://developer.mozilla.org/en-US/docs/Games/Anatomy

As well as from my understanding the draw calls won't happen until requestAnimationFrame returns as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.