[Xcb] [Bug 29561] _XReply: performance regression in latest version

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=29561

--- Comment #4 from Rami Ylimaki <rami.ylimaki at vincit.fi> 2010-12-07 01:37:37 PST ---
(In reply to comment #3)
> Possibly super niave question: Why not split xcb_poll_for_event into an
> internal and a wrapper. The wrapper would contain the whole implementation
> as-is by being a call to the internal (say xcb_check_for_buffered_events) and
> then try the file descriptor once if the check returned nada.

This is how xcb_poll_for_event() is working currently. It's calling internal
get_event() to read a buffered event before trying to read from the connection
once.

I actually implemented a public xcb_poll_for_queued_event() function that never
tries to read anything from the connection. Using this in _XReply() and
_XReadEvents() solves the excessive system call problem when requests are done
and events read in blocking fashion.

> Now xcb_wait_for_reply could also call xcb_check_for_buffered_events before
> going into its poll/wait/whatever; which will naturally already have the path
> to include the socket in its wait-for-read list. (If it doesn't already include
> the socket that xcb_poll_for_event does the non-blocking read upon, then this
> could deadlock anyway, or it is already ready to absorb the hit for unread
> events that could happen after xcb_poll_for_event EAGAIN(ed) anyway. In either
> case it has zero repeatable impact to skip the actual/expensive failed read.)
> 
> The always-oughta rules for poll safe polling are:
> (1) you have to check your pre-read data before it is safe to call poll/epoll,
> and (2) you always-oughta include all the file descriptors that can feed the
> pre-read data pool you check before a poll in the poll itself.
> 
> Every path that looks like
> "buffer_check-maybe_return-nb_read_into_buffer-maybe_return-poll-nb_read_into_buffer-maybe_return"
> is a priori wrong compared to
> "buffer_check-maybe_return-poll-nb_read_into_buffer-maybe_return".
> 
> So split out the buffer check part of xcb_poll_for_event. Now you don't need a
> whole buffer select API, you just process what you got or you poll-wait if you
> got nothing buffered. You do this buffer check for all paths, and you decide to
> read/poll only once on any path.

If I understood your comments correctly, the buffer check part has been already
separated in XCB. Read system calls aren't executed unless necessary. The
original problem comes from the fact that the current public XCB API doesn't
make it easy to implement Xlib so that excessive reading on the connection
could be avoided.

Adding something like xcb_poll_for_queued event() to the XCB public interface
seems to fix the problem. I haven't posted my patches to the mailing list yet,
I will do so when I find time for that. However, the XCB maintainers might have
other (more generic) plans regarding this problem so my patches won't be
necessarily accepted.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.