|
Description
Simon McVittie
2011-01-21 07:24:27 UTC
Some progress on this; a dbus-daemon can now use epoll! This is a work-in-progress branch which will be rebased. Note that it's currently *less* efficient than the 1.4.x code, because it allocates and frees a DBusSocketSet (secretly a DBusPollFD[] in the poll case and an epoll fd in the epoll case) on every main loop iteration. The next step is to keep the DBusSocketSet around for the lifetime of the DBusLoop, so we can reap the benefits of epoll. (In reply to comment #1) > The next step is to keep the DBusSocketSet around for the lifetime of the > DBusLoop, so we can reap the benefits of epoll. Done. Some profiling required; this may or may not in fact be a performance win. However, the tests pass. Created attachment 42531 [details] [review] [1/5] DBusSocketSet: new abstraction for struct pollfd[] or whatever Created attachment 42532 [details] [review] [2/5] Check for epoll in configure.ac Created attachment 42533 [details] [review] [3/5] Add an implementation of DBusSocketSet using epoll Created attachment 42534 [details] [review] [4/5] Add a stub _dbus_loop_toggle_watch and call it where needed Created attachment 42535 [details] [review] [5/5] Maintain the set of polled sockets over time This enables us to benefit from epoll(4): polling is now O(number of watch changes + number of active fds), whereas with poll(2) it's O(total number of fds). During testing (admittedly, a synthetic test that's somewhat biased towards epoll), this turns out to increase performance significantly: * test environment: an embedded device with many idle D-Bus services, a trivial dbus-glib service echoing messages, and a trivial dbus-glib client making 50000 blocking round-trips to it under time(1) * tests run on the normal session bus, and on an otherwise empty session bus * without Bug #33336, Bug #33342, Bug #23194 and this bug fixed: 1:45 in the normal session, 0:54 in the empty session * with those fixed: 0:56 in the normal session, 0:55 in the empty session * all times are ± half a second on repeated runs Created attachment 42905 [details] [review] [6] Choose between poll and epoll at runtime If we're compiled against glibc 2.9 or later, but then run on Linux 2.6.26 or earlier (for instance, a Debian 6 chroot on a Debian 5 host), epoll_create1 won't work and we'll have to fall back to poll anyway. Created attachment 42906 [details] [review] [7] Use epoll in a backwards-compatible way on Linux < 2.6.27 Created attachment 45390 [details] [review] [4/5 v2] Add a stub _dbus_loop_toggle_watch and call it where needed Adjusted to apply after stylistic fixes in Bug #23194. (Note that this must be applied in the sequence indicated by the attachment names: don't use git-bz. I have a public branch whose gitweb is in the URL field.) not for 1.4 Created attachment 47907 [details] [review] [PATCH 1/6] DBusSocketSet: new abstraction for struct pollfd[] or whatever In this second version of this patch, DBusSocketSet is an "abstract base class" so that when using a better OS-specific API fails, we can always fall back to _dbus_poll(). For instance, this is necessary when the "better OS-specific API" is epoll on Linux, the build machine has a modern glibc version, and the host machine either has an old kernel, is emulated in qemu (which does not support the epoll syscalls yet), or both. Created attachment 47908 [details] [review] [PATCH 2/6] Check for epoll in configure.ac Created attachment 47909 [details] [review] [PATCH 3/6] Add an implementation of DBusSocketSet using epoll Created attachment 47910 [details] [review] [PATCH 4/6] Add a stub _dbus_loop_toggle_watch and call it where needed Created attachment 47911 [details] [review] [PATCH 5/6] Maintain the set of polled sockets over time This enables us to benefit from epoll(4): polling is now O(number of watch changes + number of active fds), whereas with poll(2) it's O(total number of fds). Created attachment 47912 [details] [review] [PATCH 6/6] Use epoll in a backwards-compatible way on Linux < 2.6.27 (In reply to comment #0) > Much of the same refactoring will be needed for direct epoll, libevent, libev > and anyone else's pet API (the BSD equivalents of epoll, for instance). Google suggests that Solaris /dev/poll and *BSD kqueue were the inspiration for epoll, and should also fit into this code structure fairly well. Review of attachment 47907 [details] [review]: This looks fine. Comment on attachment 47908 [details] [review] [PATCH 2/6] Check for epoll in configure.ac Review of attachment 47908 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 47909 [details] [review] [PATCH 3/6] Add an implementation of DBusSocketSet using epoll Review of attachment 47909 [details] [review]: ----------------------------------------------------------------- This looks plausible, modulo the backwards-compatibility concerns you address in a later patch. Comment on attachment 47911 [details] [review] [PATCH 5/6] Maintain the set of polled sockets over time Review of attachment 47911 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-mainloop.c @@ +155,4 @@ > loop->watches = _dbus_hash_table_new (DBUS_HASH_INT, NULL, > free_watch_table_entry); > > + loop->socket_set = _dbus_socket_set_new (0); Surely you need a check for this returning NULL? Comment on attachment 47910 [details] [review] [PATCH 4/6] Add a stub _dbus_loop_toggle_watch and call it where needed Review of attachment 47910 [details] [review]: ----------------------------------------------------------------- This looks right, given the following patch. I trust you to have checked that you've wired it up to everything that uses watches. Comment on attachment 47912 [details] [review] [PATCH 6/6] Use epoll in a backwards-compatible way on Linux < 2.6.27 Review of attachment 47912 [details] [review]: ----------------------------------------------------------------- This covers the two backwards-compatibility concerns I came across while reviewing the other patches, so, hooray! Created attachment 53061 [details] [review] _dbus_loop_new: don't crash on OOM allocating socket set Also don't leak the socket set if allocating watches failed, or vice versa. Based on review feedback from wjt. --- I think this fixes your only criticism of this branch? (In reply to comment #26) > _dbus_loop_new: don't crash on OOM allocating socket set > > Also don't leak the socket set if allocating watches failed, or > vice versa. Based on review feedback from wjt. > > --- > > I think this fixes your only criticism of this branch? Any chance of review on this? I'd like to have this in 1.6. Comment on attachment 53061 [details] [review] _dbus_loop_new: don't crash on OOM allocating socket set Review of attachment 53061 [details] [review]: ----------------------------------------------------------------- Yup, this looks about right to me. Ship it. Fixed in master for 1.5.10 |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.