[PATCH] get rid of --enable-verbose-mode configure option in favor of DBUS_VERBOSE env variable

[Simon McVittie]

On 12/01/12 00:36, Pavel Strashkin wrote:
> I re-used "_dbus_is_verbose" function so there is no performance
> penalty at all

Sorry, but if you make sweeping statements about performance not 
accompanied by a benchmark, I'm not going to believe them - particularly 
when they don't appear likely to be true.

For your convenience, here is a simple test that shovels 10k messages 
through the dbus-daemon (requires GLib to be installed at configure 
time), and its result:

reptile% mkdir ~/build/dbus/rel ~/build/dbus/verbose
# "release build" like the tarballs do by default
reptile% ( cd ~/build/dbus/rel && ~/src/fdo/dbus/configure )
... # it configures
# equivalent to your proposed change
reptile% ( cd ~/build/dbus/verbose && ~/src/fdo/dbus/configure 
--enable-verbose-mode )
... # it configures
NOTE: building with verbose mode increases library size, may slightly 
increase security risk, and decreases performance.
reptile% make -j -l4 -C ~/build/dbus/rel
... # it compiles
reptile% make -j -l4 -C ~/build/dbus/verbose
... # it compiles
reptile% time ~/build/dbus/verbose/test/test-dbus-daemon -m perf -v
/echo/session: OK
/echo/limited: ** Message: SKIP: set DBUS_TEST_DATA to a directory 
containing valid-config-files/incoming-limit.conf
OK
~/build/dbus/verbose/test/test-dbus-daemon -m perf -v  4.49s user 1.03s 
system 61% cpu 8.962 total
reptile% time ~/build/dbus/rel/test/test-dbus-daemon -m perf -v
/echo/session: OK
/echo/limited: ** Message: SKIP: set DBUS_TEST_DATA to a directory 
containing valid-config-files/incoming-limit.conf
OK
~/build/dbus/rel/test/test-dbus-daemon -m perf -v  3.91s user 1.08s 
system 60% cpu 8.308 total

(I repeated the "rel" and "verbose" tests a few times - results are 
consistent.)

I make that an 8% reduction in throughput - in a relatively simple test 
on a dual-core laptop, not one of the more constrained environments 
where D-Bus is used, like a phone! - for merely having the ability to 
have verbose mode. People already complain about D-Bus' performance 
quite enough, without us making it worse.

 > Right now 48% of code with #ifdef,
 > 48% without and 2% with #ifdef 1. Kind of cleanup i'd say.

You're right that bits with #if 1 (or #if 0 for that matter) are stupid 
now that we have a decent revision control system, and I'd happily drop 
those; patches welcome.

The bits with #ifdef VERBOSE are those where omitting the 
_dbus_verbose() call also allows omission of a function call that isn't 
an argument to the _dbus_verbose() macro, or an otherwise-unused variable.

The bits without #ifdef VERBOSE are those where the conditionalization 
built into the _dbus_verbose() macro already has the desired result when 
verbosity isn't enabled.

Please read and understand the implementation of _dbus_verbose, and in 
particular consider the performance difference between these two 
possible implementations it could have had, before making statements 
about its performance implications. It turns out there is, in fact, a 
reason for it (our verbose mode is so ridiculously spammy, and present 
on enough fast-paths, that it really does produce a performance hit even 
when not enabled at runtime).

Regards,
     S

 > /* Possible implementation 1 (simplified - imagine it had the
 >  * obvious split between .h and .c). This is close to what we
 >  * actually have */
 > #ifdef VERBOSE
 > #   define _dbus_verbose(fmt, ...) \
 >     _dbus_real_verbose(fmt, __VA_ARGS__)
 >
 > void
 > _dbus_real_verbose (const char *fmt, ...)
 > {
 >   /* pseudocode */
 >   if (verbosity is enabled)
 >     {
 >       fprintf (stderr, fmt, va_args);
 >     }
 > }
 > #else
 > #   define _dbus_verbose(fmt, ...) do {} while (0)
 > #endif

 > /* Possible implementation 2, essentially equivalent
 >  * to -DVERBOSE all the time. Exercise to the reader:
 >  * why is this worse? */
 > void
 > _dbus_verbose (const char *fmt, ...)
 > {
 >   /* same implementation as _dbus_real_verbose above */
 > }