Wayland: Fix engine stalls while waiting frames#102674
Wayland: Fix engine stalls while waiting frames#102674akien-mga merged 1 commit intogodotengine:masterfrom
Conversation
| // `wl_display_prepare_read` and `wl_display_read`. This means, that it will | ||
| // basically be guaranteed to stay stuck in a "prepare read" state, where it | ||
| // will block any other attempt at reading the display fd, such as ours. The | ||
| // solution? Let's make sure we the mutex is locked (it should) and unblock the |
There was a problem hiding this comment.
| // solution? Let's make sure we the mutex is locked (it should) and unblock the | |
| // solution? Let's make sure the mutex is locked (it should) and unblock the |
That being said - "let's make sure the mutex is locked (it should)" sounds to me like it might already be locked and we're relocking again to be extra sure. Isn't it problematic to double-lock a mutex?
There was a problem hiding this comment.
Edit: I'm dumb, see next post.
Uh this is weird. Like, for some reason I'm extremely sure that MutexLocks are by default recursive (i.e. can be relocked on the same thread multiple times), but I don't know where I got that information.
I'm having trouble parsing the template magic so... I'm not really sure now. I can only tell you that it worked all along and there's this in core/os/mutex.cpp:
template class MutexImpl<THREADING_NAMESPACE::recursive_mutex>;
template class MutexImpl<THREADING_NAMESPACE::mutex>;
template class MutexLock<MutexImpl<THREADING_NAMESPACE::recursive_mutex>>;
template class MutexLock<MutexImpl<THREADING_NAMESPACE::mutex>>;But I'm not sure if that confirms that a plain MutexLock corresponds to MutexLock<MutexImpl<THREADING_NAMESPACE::recursive_mutex>>
There was a problem hiding this comment.
Holy moly just as I pressed enter in #core the answer popped up in my mind. I should've slept over this lol.
I now know why I'm sure. MutexLock is just a fancy RAII wrapper but the thing it locks depends on you, that's why it's called a lock. WaylandThread uses a plain and simple Mutex which maps to:
core/os/mutex.h
using Mutex = MutexImpl<THREADING_NAMESPACE::recursive_mutex>; // Recursive, for general useThat's where it's written. So yea, we can lock it as much as we want, as long as it's the same thread obviously, otherwise it's going to wait.
FTR there's also this in the aformentioned header:
using BinaryMutex = MutexImpl<THREADING_NAMESPACE::mutex>; // Non-recursive, handle with careAnd that would be problematic to relock, even in the same thread.
Docs for reference: https://en.cppreference.com/w/cpp/thread/recursive_mutex
9f1b281 to
cbd68eb
Compare
There were two edge cases in the frame waiting logic (aka manual frame throttling or emulated vsync) which would cause the editor to stall in one way or another: 1. Waiting right after starting the editor would cause a deadlock between both threads until something happened in the Wayland event queue, in turn unblocking the Wayland thread and kickstartin the whole thing; 2. Starting the editor (and probably other long-loading stuff) without low consumption mode would suspend the window and never commit its surfaces, thus never signaling the compositor that we want frame events.
|
Thanks! |
Originally part of #101774.
There were two edge cases in the frame waiting logic (aka manual frame throttling or emulated vsync) which would cause the editor to stall in one way or another:
Waiting right after starting the editor would cause a deadlock between both threads until something happened in the Wayland event queue, in turn unblocking the Wayland thread and kickstartin the whole thing;
Starting the editor (and probably other long-loading stuff) without low consumption mode would suspend the window and never commit its surfaces, thus never signaling the compositor that we want frame events.
I'm kinda surprised nobody stumbled on the second one, it's quite bad when it happens.
Obviously, as with any good Wayland PR, most of the patch is actually just comments, so that this does not happen anymore... Hopefully :P