Port X11 backend from xcb to x11rb. #1025

jneem · 2020-06-09T20:31:02Z

This ports the X11 backend from the xcb crate to the x11rb crate, as discussed in #989. Overall, I have a positive impression of x11rb, as compared to xcb: it's actively maintained, it uses actual rust enums in many places, and it doesn't need unsafe for event handling. Also, it supports the present extension (a blocker for #989) and the xinput extension (which presumably we'll need at some point), unlike xcb.

luleyleo · 2020-06-10T10:11:26Z

/bloat

github-actions · 2020-06-10T10:17:25Z

🗜 Bloat check ⚖️

Comparing 76b9234 against 0085e16

target	old size	new size	difference
target/release/examples/calc	1.18 MB	1.18 MB	---
target/release/examples/scroll_colors	1.16 MB	1.16 MB	---
target/release/examples/multiwin	1.18 MB	1.18 MB	---
target/release/examples/flex	1.54 MB	1.54 MB	---
target/release/examples/styled_text	1.29 MB	1.29 MB	---
target/release/examples/custom_widget	1.1 MB	1.1 MB	---

jneem · 2020-06-10T15:42:52Z

I think bloat only does macos, so it won't pick up changes in x11. I did the comparison myself, though, and got this (sizes in KB):

target	old	new
calc	1040	1140
custom_widget	988	1088
flex	1380	1484
multiwin	1060	1160
scroll_colors	1032	1132
styled_text	1156	1260

luleyleo · 2020-06-10T18:32:26Z

Thanks a lot for doing the comparison, I completely forgot about that bot being macOS only.
But 100 KB larger binaries is quite a bit of bloat, I wonder where that comes from 🤔

luleyleo

This looks like a pretty nice diff overall 👍

druid-shell/src/platform/x11/application.rs

druid-shell/src/platform/x11/util.rs

druid-shell/src/platform/x11/window.rs

jneem · 2020-06-10T20:39:52Z

I had a look using cargo bloat, and there seems to be quite a bit of code for parsing X11 replies (given by &[u8]) to rust structs. xcb avoids this by leaning on the parsing code in libxcb and then just using FFI to access the structs.

cmyr · 2020-06-12T01:34:02Z

it should be possible to adapt the bloat stuff to take a backend as an argument or something 🤔

jneem · 2020-06-12T02:13:47Z

In this case, we would also need it to take a feature flag...

…

On Thu, Jun 11, 2020, 20:34 Colin Rofls ***@***.***> wrote: it should be possible to adapt the bloat stuff to take a backend as an argument or something 🤔 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1025 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALBRMKWMS3I4XZURAMTRW3RWGAZNANCNFSM4NZX5CHQ> .

psychon · 2020-06-15T14:48:28Z

But 100 KB larger binaries is quite a bit of bloat, I wonder where that comes from 🤔

xcb avoids this by leaning on the parsing code in libxcb and then just using FFI to access the structs.

libxcb is basically a big std::mem::transmute (well, it is C code... it's a pointer cast). The whole idea of libxcb is to describe the X11 on-the-wire protocol as C structs so that "no actual parsing" happens / the C code directly reads the on-the-wire-representation. (This is also the origin of most of the API weirdness.) (This also means that there is not much length checking and a malicious X11 server could send a packet of length 42 containing a list which claims to have a million entries.)

Thus, x11rb really does a lot more than libxcb. If you want to call that "bloat" or not is up for debate. :-)

(By the way, x11rb doing actual parsing is why it supports more extension than the XCB crate; some of the on-the-wire stuff does not really map to C structs, but since x11rb "invents" its own representation, it can work with these more easily.)

TL;DR: Even when also considering the size of libxcb.so and all the libxcb-foo.so, x11rb will likely result in larger code.

jneem · 2020-06-15T23:50:55Z

It's not that I think 100kb is excessive, but I do think it might be possible (if not a high priority) to improve. According to cargo bloat, Event::parse is more than 9kb of code (this is only with randr). Then there's also a bunch of things like KeyPressEvent::try_parse at around the 3kb mark. I bet that rustc is doing a lot of inlining, and that the error-handling code could be de-duplicated.

xStrom

I just skimmed parts of this for now. Mostly I was looking at x11rb itself and I think the general idea of migrating to that library is a good one. 👍

CHANGELOG.md

xStrom · 2020-06-18T18:07:22Z

druid-shell/src/platform/x11/application.rs

@@ -74,59 +72,61 @@ struct State {

 impl Application {
    pub fn new() -> Result<Application, Error> {
-        let (conn, screen_num) = Connection::connect_with_xlib_display()?;
+        let (conn, screen_num) = XCBConnection::connect(None)?;


I haven't really investigated this stuff with either library, but is this an equivalent port? The connect_with_xlib_display docs say that going through XLib is needed for OpenGL support. I'm not sure if cairo uses OpenGL but if it does - does the new connection also provide that?

To my knowledge Cairo does not use OpenGL by default. There is an experimental feature for it but otherwise it uses the platform library (X11 on Linux). So it should be fine if there is no OpenGL available, unless we want to use it ourselves.

I suspect that we will want OpenGL support eventually. If not for cairo, then for #891-related stuff. But it looks like it will need some work on the x11rb side.

I guess you will want to use the x11 crate for this. (The following "talks C", I haven't looked at the Rust bindings of any of this.)

create you Display* via x11.

Get the underlying XCB Connection via XGetXCBConnection. This gives you an xcb_connection_t*.

This pointer can be given to x11rb::XCBConnection::from_raw_xcb_connection(dpy, false).

If you want to handle events through x11rb and libxcb, you should call XSetEventQueueOwner(dpy, XCBOwnsEventQueue). Otherwise, libX11 might randomly eat your events / move them to its own event queue.

One you drop / XCloseDisplay the Display*, any further use of the XCBConnection might cause demons to fly through your nose, so be careful (by the way: Similar fun can result with cairo, but cairo makes it much easier to accidentally continue using the connection after it was closed / freed.).

Congratulations, you now have a single X11 connection which you can access through all of libX11, libxcb and x11rb.

API-wise, I do not see anything that could be added to x11rb. The two functions I mentioned above live in libX11.so and libX11-xcb.so and x11rb certainly has no business depending on them.

Edit: This is also what connect_with_xlib_display does (minus the setting of the event queue owner...?) https://github.com/rtbo/rust-xcb/blob/69a9d0126d40e7e3a9b354c07fbf31a81443b105/src/base.rs#L637-L640

Thanks for the detailed information! I'd prefer not to block this PR on OpenGL support, so I've just dropped in a comment for now.

luleyleo

The broken link aside, this looks great!
Happy to be using a well maintained and Rusty library from now on 😄

luleyleo · 2020-06-19T18:03:30Z

druid-shell/src/platform/x11/application.rs

+        // libxcb, you should call XSetEventQueueOwner(dpy, XCBOwnsEventQueue). Otherwise, libX11
+        // might randomly eat your events / move them to its own event queue.
+        //
+        // https://github.com/xi-editor/druid/pull/1025/files/76b923417183bd103f61e56b56a56474b7417cec#r442777892


This link is broken for me.

Thanks, fixed, and I'll merge when CI lets me. Definitely nice to be using a library that is both rusty and actively maintained to the point where the maintainer drops by and tells us what we should be doing better 😄

luleyleo added S-needs-review waits for review shell/x11 concerns the X11 backend labels Jun 9, 2020

luleyleo requested changes Jun 10, 2020

View reviewed changes

luleyleo added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Jun 10, 2020

luleyleo added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Jun 11, 2020

psychon mentioned this pull request Jun 16, 2020

Can we get smaller binary code? psychon/x11rb#488

Open

xStrom reviewed Jun 18, 2020

View reviewed changes

jneem added 4 commits June 19, 2020 11:36

Port X11 backend from xcb to x11rb.

869c639

Add changelog entry.

184312c

Review comments.

0e02b08

Update x11rb.

38b6248

jneem requested a review from luleyleo June 19, 2020 17:50

luleyleo approved these changes Jun 19, 2020

View reviewed changes

Fix broken link.

b0d3afd

jneem merged commit 6029920 into linebender:master Jun 19, 2020

xStrom removed the S-needs-review waits for review label Jun 20, 2020

jneem mentioned this pull request Jun 20, 2020

X11 present #989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port X11 backend from xcb to x11rb. #1025

Port X11 backend from xcb to x11rb. #1025

jneem commented Jun 9, 2020

luleyleo commented Jun 10, 2020

github-actions bot commented Jun 10, 2020

jneem commented Jun 10, 2020

luleyleo commented Jun 10, 2020

luleyleo left a comment

jneem commented Jun 10, 2020

cmyr commented Jun 12, 2020

jneem commented Jun 12, 2020 via email

psychon commented Jun 15, 2020

jneem commented Jun 15, 2020

xStrom left a comment

xStrom Jun 18, 2020

luleyleo Jun 18, 2020

jneem Jun 18, 2020

psychon Jun 19, 2020 •

edited

Loading

jneem Jun 19, 2020

luleyleo left a comment

luleyleo Jun 19, 2020

jneem Jun 19, 2020

Port X11 backend from xcb to x11rb. #1025

Port X11 backend from xcb to x11rb. #1025

Conversation

jneem commented Jun 9, 2020

luleyleo commented Jun 10, 2020

github-actions bot commented Jun 10, 2020

🗜 Bloat check ⚖️

jneem commented Jun 10, 2020

luleyleo commented Jun 10, 2020

luleyleo left a comment

Choose a reason for hiding this comment

jneem commented Jun 10, 2020

cmyr commented Jun 12, 2020

jneem commented Jun 12, 2020 via email

psychon commented Jun 15, 2020

jneem commented Jun 15, 2020

xStrom left a comment

Choose a reason for hiding this comment

xStrom Jun 18, 2020

Choose a reason for hiding this comment

luleyleo Jun 18, 2020

Choose a reason for hiding this comment

jneem Jun 18, 2020

Choose a reason for hiding this comment

psychon Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

jneem Jun 19, 2020

Choose a reason for hiding this comment

luleyleo left a comment

Choose a reason for hiding this comment

luleyleo Jun 19, 2020

Choose a reason for hiding this comment

jneem Jun 19, 2020

Choose a reason for hiding this comment

psychon Jun 19, 2020 •

edited

Loading