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

Adds an idle loop to the x11 backend, and uses it for repainting. #1072

Merged
merged 11 commits into from
Jul 18, 2020
Merged

Adds an idle loop to the x11 backend, and uses it for repainting. #1072

merged 11 commits into from
Jul 18, 2020

Conversation

jneem
Copy link
Collaborator

@jneem jneem commented Jun 30, 2020

Fixes #932 and #935.

@jneem jneem marked this pull request as draft June 30, 2020 18:44
@jneem jneem marked this pull request as ready for review June 30, 2020 18:59
@jneem jneem added the S-needs-review waits for review label Jun 30, 2020
Copy link
Contributor

@psychon psychon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments. Feel free to ignore whichever parts you want to ignore. I am not quite confused about your thread-safety requirements / use of threads.

druid-shell/src/platform/x11/application.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
@luleyleo luleyleo added the shell/x11 concerns the X11 backend label Jul 2, 2020
@jneem
Copy link
Collaborator Author

jneem commented Jul 2, 2020

Thanks for the comments! Regarding the threading: the intention (although it probably isn't correct right now) is to do the interaction with X11 only the main thread, but to support threads for other things. For example, IdleHandle is Send + Sync, so you can hand it to another thread. That thread can use the handle to queue a callback, but then that callback will be run on the main thread. Hopefully this explains the use of pipes.

Regarding the interaction with x11rb, could you expand on what the thread-safety issues are? I noticed that XCBConnection is Send + Sync, but maybe it shouldn't be? Or maybe a better question is: if I wrap our XCBConnection in a struct that is !Sync, will that avoid all potential threading issues. (Should I make it !Send also?) Actually, on windows our WindowHandle type is already !Send and !Sync, so this should probably be replicated on other platforms to avoid portability issues.

@psychon
Copy link
Contributor

psychon commented Jul 2, 2020

Regarding the interaction with x11rb, could you expand on what the thread-safety issues are?

The issue is that libxcb and XCBConnection have an internal queue of pending events that were read from the socket, but not yet passed to the caller. Thus "an event is (potentially) pending" is not the same as "the socket is readable".

Let's say that the main thread is "almost done" with a main loop iteration and is about to call poll(). We now have a race condition and another thread gets to run some code.
This other thread now does conn.get_input_focus()?.reply(). Thus, it sends a request and waits for its reply. To wait for the reply, it has to read X11 packages from the connection until it "finds" the reply to the request. Any events that it reads ends up in the internal queue.
Now the main thread continues with poll() and blocks even though there are pending events.

Hopefully this makes the issue clearer.

I noticed that XCBConnection is Send + Sync, but maybe it shouldn't be?

Well, you could do what Qt5 is doing: You spawn a thread that does loop { let event = conn.wait_for_event(); send_to_main_loop(event); }. This wait_for_event() is made to work correctly by x11rb (via a condition variable, std::sync::Condvar). Thus, in this scenario, everything works correctly and the connection really is Sync.

The issue is that "poll() on the socket to know when to call poll_for_event()" is not thread-safe, but all the APIs themselves are thread-safe.

Does that make sense? Do you have any ideas how x11rb could work differently?

@jneem
Copy link
Collaborator Author

jneem commented Jul 2, 2020

Thanks, that makes a lot of sense. The only thing I would suggest to change in x11rb would be to have an explanation like the one you gave in the API docs. I've been kind of spoiled by rust over the last couple years, to the extent that if something Send + Sync, I just assume that there are no threading issues...

As far as druid is concerned, it sounds to me like everything will be fine as long as we restrict the xcb connection to one thread. As far as I can tell, access to the connection is restricted to the WindowHandle and the cairo context that we create; I've marked the WindowHandle as !Send and !Sync, and the cairo context is exposed as piet::CairoRenderContext, which is already !Send and !Sync. So I think the threading issue should be ok now.

Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to let this sit around for so long.
Looks pretty good so already, got a few more questions and suggestions.

druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/application.rs Outdated Show resolved Hide resolved
@luleyleo luleyleo added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Jul 14, 2020
jneem and others added 6 commits July 14, 2020 15:49
Co-authored-by: Leopold Luley <git@leopoldluley.de>
Co-authored-by: Leopold Luley <git@leopoldluley.de>
Co-authored-by: Leopold Luley <git@leopoldluley.de>
Co-authored-by: Leopold Luley <git@leopoldluley.de>
@jneem jneem added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Jul 17, 2020
@jneem
Copy link
Collaborator Author

jneem commented Jul 17, 2020

Ok, I think the review comments are addressed now.

@luleyleo luleyleo removed the S-needs-review waits for review label Jul 17, 2020
@jneem jneem merged commit 5afab1b into linebender:master Jul 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shell/x11 concerns the X11 backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

X11 shell uses sleep to sync rendering.
3 participants