[Xcb] [RFC PATCH] New XCB socket handoff mechanism for Xlib/XCB and other libraries

[Ian Osgood]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


On Mar 15, 2008, at 9:36 PM, Jamey Sharp , Josh Triplett wrote:

> Libraries like Xlib/XCB, some XCB language bindings, and potentially
> others have a common problem: they want to share the X connection with
> XCB. This requires coordination of request sequence numbers. XCB
> currently has an Xlib-specific lock, and allows Xlib to block XCB from
> making requests. The attached patches to XCB replace that lock with a
> handoff mechanism, xcb_take_socket, allowing external code to ask XCB
> for permission to take over the write side of the socket and send raw
> data with xcb_writev. The caller of xcb_take_socket must supply a
> callback which XCB can call when it wants the write side of the socket
> back to make a request. This callback synchronizes with the external
> socket owner, flushes any output queues if appropriate, and then  
> returns
> the sequence number of the last request sent over the socket.
>
> The attached patch to Xlib/XCB converts it to use the new handoff
> mechanism. Previously, Xlib/XCB used XCB's Xlib lock to prevent XCB  
> from
> sending requests between calls to Xlib's LockDisplay and UnlockDisplay
> macros. Xlib/XCB then sent all of its requests using XCB's
> xcb_send_request, and had to flush its requests when unlocking the
> display.
>
> Using the new handoff mechanism, we replaced much of the existing
> Xlib/XCB implementation with the use of xcb_take_socket to take
> ownership of the write side of the X connection socket, and a
> return_socket callback which writes any outstanding requests with
> xcb_writev. This approach allows Xlib/XCB to use the same buffering as
> traditional Xlib did. In particular, programs which use Xlib/XCB and
> never make XCB calls will never need to hand the socket back to  
> XCB, and
> vice versa.
>
> This allows us to discard large quantities of synchronization code  
> from
> Xlib/XCB, together with the synchronization bugs present in that code.
> Several test cases which previously failed now work perfectly,  
> including
> multi-threaded ico. In addition, the infamous locking correctness
> assertions, triggered when double-locking or when unlocking without a
> previous lock, no longer exist, because Xlib/XCB no longer has any
> reason to care more about application locking than traditional Xlib
> does.
>
> Let's repeat that for emphasis: the locking assertions *no longer
> exist*. Existing Xlib applications with buggy locking will now work  
> just
> as well, or as badly, as they did with traditional Xlib.
>
> Furthermore, the handoff approach provides great improvements to
> performance. Results from x11perf's XNoOp test, which represented the
> worst case for the lock-based Xlib/XCB:
>
> Traditional Xlib:       average 19100000/sec
> Lock-based Xlib/XCB:    average  3350000/sec
> Handoff-based Xlib/XCB: average 17400000/sec
>
> Thus, for no-ops, the handoff mechanism provides more than a 4x  
> speedup
> to Xlib/XCB, bringing Xlib/XCB within 9% of traditional Xlib no-op
> performance. Of course, real-world workloads do not use no-op, so your
> mileage may vary. In particular, since no-ops represent the worst  
> case,
> we expect real workloads to more closely match the performance of
> traditional Xlib.
>
> Finally, we anticipate that this new handoff mechanism will prove  
> useful
> for more than just Xlib/XCB. In particular, several authors or  
> potential
> authors of language bindings based on the XML-XCB protocol  
> descriptions
> have expressed an interest in implementing the protocol code in their
> native language, rather than simply using a foreign function interface
> (FFI) to call XCB's protocol layer. In the past, we have recommended
> against this, because then other X client code in the same process  
> (such
> as other libraries for which the language uses an FFI) could not share
> the same X connection. Those language bindings could now use XCB's new
> handoff mechanism when sending requests from their own protocol layer,
> and rely only on XCB's transport layer.
>
> We blame Keith Packard for the fundamental idea of the handoff
> mechanism, and for most of its advantages.
>
> Please review and provide comments. We hope to commit these patches  
> and
> release new versions of XCB and Xlib/XCB soon.
>
> - Josh Triplett and Jamey Sharp<xcb-0001-Remove-libxcb-xlib-and- 
> xcbxlib.h.patch><xcb-0002-Inline-_xcb_lock_io-_xcb_unlock_io-and- 
> _xcb_wait_i.patch><xcb-0003-Use-sequence-number-ranges-in-pending- 
> replies.patch><xcb-0004-Support-handing-off-socket-write-permission- 
> to-exter.patch><xcb-0005-Bump-version-number- 
> to-1.2.patch><xlib-0001-Use-XCB-s-new-socket-handoff-mechanism- 
> rather-than-t.patch>_______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb

It wasn't obviously clear to me what some of the extra fields were,  
like socket_moving and real_bufmax. Perhaps you could capture some of  
your tricky threading design in code commentary?

Is there any danger that one workaround would overwrite another? Do  
the workarounds need to be masks rather than an enum? Or separate enums?

Why "offsetof" but "container_of" with an underscore? As local  
macros, should they be XCB_ALLCAPS, like the preceding  
XCB_SEQUENCE_COMPARE? If they are common practice, is there some  
other header you could include to get them?

How does _xcb_in_expect_reply differ from _xcb_in_expect_replies?

Does Xlib configure.ac still need both X11_REQUIRES and  
X11_EXTRA_DEPS for xcb?

In OpenDis.c, this looks better to me:
+#if USE_XCB
+    dpy->bufmax = dpy->buffer;
+    dpy->xcb->real_bufmax = dpy->buffer + conn_buf_size;
+#else
      dpy->bufmax = dpy->buffer + conn_buf_size;
+#endif

Why did the dpy->bigreq_size determination move?

Ian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iD8DBQFH3Z08HDwFgzc3zyIRAjiBAKCFg6G/iA/OTPvjimhsElMs2NgFNgCfSgE/
dteiQGb6TrvMjpOSPKk4gAY=
=vMUz
-----END PGP SIGNATURE-----