Bug 33337 - poll more efficiently, optionally using epoll or libevent or something
Summary: poll more efficiently, optionally using epoll or libevent or something
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium enhancement
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL: http://cgit.freedesktop.org/~smcv/dbu...
Whiteboard: NB#197191
Keywords: patch
Depends on: 23194 33126 33336
Blocks: dbus-1.5
  Show dependency treegraph
 
Reported: 2011-01-21 07:24 UTC by Simon McVittie
Modified: 2012-02-07 07:21 UTC (History)
10 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[1/5] DBusSocketSet: new abstraction for struct pollfd[] or whatever (18.92 KB, patch)
2011-01-26 09:12 UTC, Simon McVittie
Details | Splinter Review
[2/5] Check for epoll in configure.ac (1.39 KB, patch)
2011-01-26 09:12 UTC, Simon McVittie
Details | Splinter Review
[3/5] Add an implementation of DBusSocketSet using epoll (9.42 KB, patch)
2011-01-26 09:12 UTC, Simon McVittie
Details | Splinter Review
[4/5] Add a stub _dbus_loop_toggle_watch and call it where needed (7.82 KB, patch)
2011-01-26 09:13 UTC, Simon McVittie
Details | Splinter Review
[5/5] Maintain the set of polled sockets over time (11.69 KB, patch)
2011-01-26 09:13 UTC, Simon McVittie
Details | Splinter Review
[6] Choose between poll and epoll at runtime (18.83 KB, patch)
2011-02-03 10:34 UTC, Simon McVittie
Details | Splinter Review
[7] Use epoll in a backwards-compatible way on Linux < 2.6.27 (2.29 KB, patch)
2011-02-03 10:34 UTC, Simon McVittie
Details | Splinter Review
[4/5 v2] Add a stub _dbus_loop_toggle_watch and call it where needed (7.82 KB, patch)
2011-04-07 10:01 UTC, Simon McVittie
Details | Splinter Review
[PATCH 1/6] DBusSocketSet: new abstraction for struct pollfd[] or whatever (23.09 KB, patch)
2011-06-13 09:31 UTC, Simon McVittie
Details | Splinter Review
[PATCH 2/6] Check for epoll in configure.ac (1.46 KB, patch)
2011-06-13 09:32 UTC, Simon McVittie
Details | Splinter Review
[PATCH 3/6] Add an implementation of DBusSocketSet using epoll (11.69 KB, patch)
2011-06-13 09:32 UTC, Simon McVittie
Details | Splinter Review
[PATCH 4/6] Add a stub _dbus_loop_toggle_watch and call it where needed (7.90 KB, patch)
2011-06-13 09:32 UTC, Simon McVittie
Details | Splinter Review
[PATCH 5/6] Maintain the set of polled sockets over time (11.76 KB, patch)
2011-06-13 09:33 UTC, Simon McVittie
Details | Splinter Review
[PATCH 6/6] Use epoll in a backwards-compatible way on Linux < 2.6.27 (2.31 KB, patch)
2011-06-13 09:33 UTC, Simon McVittie
Details | Splinter Review
_dbus_loop_new: don't crash on OOM allocating socket set (1.04 KB, patch)
2011-11-02 08:55 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2011-01-21 07:24:27 UTC
poll(2) isn't very efficient when you have a lot of file descriptors, most of which are idle, and the way we use it in DBusLoop (and hence dbus-daemon) is rather brute-force - on every main loop iteration, we iterate through all watches to populate an array of active DBusPollFD structs, do the poll(), then throw away the array.

Maemo/MeeGo uses D-Bus pretty heavily, so we'd like something that scales better. In particular, Linux epoll looks promising.

I considered using libev, but was put off by its error handling basically being abort(), and the fact that it measures timestamps in floating-point (which won't work fast on ARM, and seems rather perverse when both libdbus and the underlying syscalls use integers counting milliseconds).

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). I'm working on this at the moment.
Comment 1 Simon McVittie 2011-01-25 11:03:03 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.
Comment 2 Simon McVittie 2011-01-26 09:11:17 UTC
(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.
Comment 3 Simon McVittie 2011-01-26 09:12:05 UTC
Created attachment 42531 [details] [review]
[1/5] DBusSocketSet: new abstraction for struct pollfd[] or whatever
Comment 4 Simon McVittie 2011-01-26 09:12:26 UTC
Created attachment 42532 [details] [review]
[2/5] Check for epoll in configure.ac
Comment 5 Simon McVittie 2011-01-26 09:12:45 UTC
Created attachment 42533 [details] [review]
[3/5] Add an implementation of DBusSocketSet using epoll
Comment 6 Simon McVittie 2011-01-26 09:13:00 UTC
Created attachment 42534 [details] [review]
[4/5] Add a stub _dbus_loop_toggle_watch and call it where needed
Comment 7 Simon McVittie 2011-01-26 09:13:33 UTC
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).
Comment 8 Simon McVittie 2011-01-27 09:11:14 UTC
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
Comment 9 Simon McVittie 2011-02-03 10:34:07 UTC
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.
Comment 10 Simon McVittie 2011-02-03 10:34:35 UTC
Created attachment 42906 [details] [review]
[7] Use epoll in a backwards-compatible way on Linux < 2.6.27
Comment 11 Simon McVittie 2011-04-07 10:01:36 UTC
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.)
Comment 12 Simon McVittie 2011-04-07 10:02:57 UTC
not for 1.4
Comment 13 Simon McVittie 2011-06-13 09:31:50 UTC
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.
Comment 14 Simon McVittie 2011-06-13 09:32:11 UTC
Created attachment 47908 [details] [review]
[PATCH 2/6] Check for epoll in configure.ac
Comment 15 Simon McVittie 2011-06-13 09:32:37 UTC
Created attachment 47909 [details] [review]
[PATCH 3/6] Add an implementation of DBusSocketSet using epoll
Comment 16 Simon McVittie 2011-06-13 09:32:56 UTC
Created attachment 47910 [details] [review]
[PATCH 4/6] Add a stub _dbus_loop_toggle_watch and call it where  needed
Comment 17 Simon McVittie 2011-06-13 09:33:17 UTC
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).
Comment 18 Simon McVittie 2011-06-13 09:33:39 UTC
Created attachment 47912 [details] [review]
[PATCH 6/6] Use epoll in a backwards-compatible way on Linux <  2.6.27
Comment 19 Simon McVittie 2011-06-13 09:45:10 UTC
(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.
Comment 20 Will Thompson 2011-08-25 11:09:38 UTC
Review of attachment 47907 [details] [review]:

This looks fine.
Comment 21 Will Thompson 2011-10-20 03:57:08 UTC
Comment on attachment 47908 [details] [review]
[PATCH 2/6] Check for epoll in configure.ac

Review of attachment 47908 [details] [review]:
-----------------------------------------------------------------

Looks good.
Comment 22 Will Thompson 2011-10-20 04:25:58 UTC
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 23 Will Thompson 2011-10-20 06:23:00 UTC
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 24 Will Thompson 2011-10-20 06:26:32 UTC
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 25 Will Thompson 2011-10-20 06:27:54 UTC
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!
Comment 26 Simon McVittie 2011-11-02 08:55:19 UTC
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?
Comment 27 Simon McVittie 2012-02-06 05:21:27 UTC
(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 28 Will Thompson 2012-02-06 06:15:15 UTC
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.
Comment 29 Simon McVittie 2012-02-07 07:21:31 UTC
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.