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

The threaded glium+winit example seems to be broken #1051

Open
pedrocr opened this issue Aug 22, 2017 · 8 comments
Open

The threaded glium+winit example seems to be broken #1051

pedrocr opened this issue Aug 22, 2017 · 8 comments

Comments

@pedrocr
Copy link
Contributor

pedrocr commented Aug 22, 2017

I've been adapting the glium+winit threaded example to chimper and input/refresh handling seems to have some brokenness to it. First the needs_update logic seems wrong:

if events.is_empty() || !needs_update {

This should probably be if events.is_empty() && !needs_update to not block on events when we've decided a new update is needed. And then there seems to be a race condition between the conrod and main threads. Here's the sequence of events.

MAIN: Received an event and passed it on to conrod
CONROD: Processed the event and issued a new draw and an event loop wakeup()
MAIN: works on the event handling some more
MAIN: blocks on event loop because the wakeup didn't come while it was blocked

Replacing event_loop.run_forever() with event_loop.poll_events() in the main thread works fine. I've tried moving the event loop into it's own thread that all it does is process events while the blocking in the main thread happens only on the render channel but that has it's own set of issues (conrod::backend::winit::convert_event() blocks, seems slow).

Any ideas on how to make this more robust?

@mitchmindtree
Copy link
Contributor

Hmm the example seems to be working fine for me. Do you get this strange behaviour when running the all_winit_glium_threaded example itself? Or do you only get it in your own code? Could you be more specific in terms off what bugs you are seeing?

Also I'm not quite sure where the race condition is that you are suggesting? The conrod thread only calls wakeup on the EventsLoopProxy after it send the primitives for drawing, so there should always be an Awakened event waiting following each submission of graphics primitives.

Replacing event_loop.run_forever() with event_loop.poll_events() in the main thread works fine.

Yes, but this also means that the main thread will continually loop at ~60fps and will still consume energy when the app is idle (this is probably fine for realtime games, but probably not OK for simple GUI apps).

I've tried moving the event loop into it's own thread

Note that this won't work on macOS as for "historic reasons" the event loop can only be polled on the main thread, so it's best to leave the main event loop polling on the main thread if you're hoping to make your app multi-platform (super frustrating, I know!).

@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 22, 2017

Do you get this strange behaviour when running the all_winit_glium_threaded example itself? Or do you only get it in your own code? Could you be more specific in terms off what bugs you are seeing?

I get it in my code because it does more stuff in the main loop. But I believe the issue is there in the example as well. What happens for me is that I run this in my main loop:

https://github.com/pedrocr/chimper/blob/a088385116cff5eb857cea0669b30d6752edfaa8/src/window.rs#L188-L189

to swap images in the map and then issue a redraw. The problem is that it takes long enough that the wakeup is lost.

The conrod thread only calls wakeup on the EventsLoopProxy after it send the primitives for drawing, so there should always be an Awakened event waiting following each submission of graphics primitives.

The problem is that Awakened is not an event. If you issue 100 wakeup() calls in quick succession you won't get 100 Awakened events because when the wakeup() is called when the event loop is not blocked it does nothing.

Yes, but this also means that the main thread will continually loop at ~60fps and will still consume energy when the app is idle (this is probably fine for realtime games, but probably not OK for simple GUI apps).

Yep, which is what I'm very actively trying to avoid.

for "historic reasons" the event loop can only be polled on the main thread

That plus the other issues makes me abandon this option as well.

@mitchmindtree
Copy link
Contributor

The problem is that it takes long enough that the wakeup is lost

The problem is that Awakened is not an event

Ahh I see, my understanding was that there should always be at least one Awakened event emitted if an EventLoopProxy called wakeup whether or not the loop is currently blocking or not. I believe this is the case for the macos backend, I thought it was also the case for x11 and wayland but perhaps I'm wrong. I think I'll open an issue at winit to clarify this behaviour.

For now, I guess one way to avoid this would be to only enter run_forever if there are no primitives already waiting to be rendered. E.g. something like

pending_primitives.extend(render_rx.try_iter().last());
if pending_primitives.is_empty() {
    events_loop.run_forever(|event| {
        ...
    });
}
if let Some(primitives) = pending_primitives.drain(..).chain(render_rx.try_iter()).last() {
    draw(...)
}

@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 22, 2017

Ahh I see, my understanding was that there should always be at least one Awakened event emitted if an EventLoopProxy called wakeup whether or not the loop is currently blocking or not.

In my testing it doesn't seem to be the case but I may be looking at things wrong.

For now, I guess one way to avoid this would be to only enter run_forever if there are no primitives already waiting to be rendered.

I considered something like that but in reality it's still a race, just less of a window to hit the issue. Between is_empty() and run_forever() you may have received a wakeup() that is ignored and you're still blocked. It's just much harder to trigger as right now you have a 16ms+ window to do it in.

@mitchmindtree
Copy link
Contributor

In my testing it doesn't seem to be the case but I may be looking at things wrong.

What platform are you on? I might be able to look at the winit src to check for your backend as I'm familiar with a fair bit of it.

In the meantime perhaps you could setup a minimal test case for this behaviour by spawning a secondary thread with the proxy and blocking on main until a message is received saying that the wakeup has been called before entering run_forever and seeing if Awakened is still received.

It's a bit tricky for me to help much further as I still don't know exactly what the strange behaviour that you're noticing is.

@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 22, 2017

I'm on X11 (Ubuntu 16.04). If you want to replicate this yourself you can compile this chimper version:

https://github.com/pedrocr/chimper/tree/20ee2f70e31fba1fdd861aa44d7de17b70a4dff6

and then run it in a directory that has images in it. Then move the mouse to the file list and select an image and stop moving the mouse (or generating any other event). You can then wait there indefinitely without the image showing up. Once you move the mouse (or generate another event) the window refreshes and you have the image.

Your test for Awakened also seems interesting.

@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 22, 2017

Here's an attempt at a minimal test case:

https://gist.github.com/pedrocr/027af24858d6bc984b2c8f01ad4ee3ab

I may be missing something but I believe this should loop forever getting Awakened events. On my machine this wakes twice and then blocks forever.

Edit: and calling the wakeup() from another thread makes no difference.

https://gist.github.com/pedrocr/63d719fac523ffe5d4d1c085312e9f6d

@pedrocr
Copy link
Contributor Author

pedrocr commented Aug 27, 2017

I've added a simplified example to chimper:

https://github.com/pedrocr/chimper/blob/3c8d39e536703ae6b8579f8379721df163efcdfd/src/bin/test_awakened.rs

Here's what I get (on Ubuntu 16.04.3):

$ cargo build --release
# (...) all the compiling goes on
$ ./target/release/test_awakened 
Awakening num 1
Draw num 1

At this point the program hangs forever when the expected behavior was for it to keep looping forever.

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

No branches or pull requests

2 participants