Bug 38287

Created attachment 47948 [details] [review]
[PATCH 1/2] DBusMessage: don't redundantly store byte order, ask the  DBusHeader

Created attachment 47949 [details] [review]
[PATCH 2/2] DBusHeader: only store byte-order in the fixed-length  header

Review of attachment 47948 [details] [review]:

Looks good.

Review of attachment 47949 [details] [review]:

Looks good.

I'm just concerned that this might be introducing a bit of function call overhead to get the byte order. In theory, the byte order should never change once the DBusMessage is created, so it should be cached locally. 

The only case I can think of for writing something outside of the local byte order is when the bus is appending stuff to a message it's forwarding.

(In reply to comment #4)
> The only case I can think of for writing something outside of the local byte
> order is when the bus is appending stuff to a message it's forwarding.

... like the sender's unique name, which it inserts into every single message? :-(

(Although that's a bug, which I'll file now if not already open: message senders should write their own unique name (if set) into their messages, so that the bus daemon just needs to validate that they have done so. For backwards compatibility it will need a slow path to overwrite it if the sender was lying, although D-Bus 2.0 - if it ever happens - should reject such messages.)

DBusMessageIter forces the message to be byteswapped into local order before opening the iterator, so you never append to the body of a foreign-endian message.

I haven't actually verified that manipulating the headers causes a similar byteswap, but if it doesn't, it probably should.

(In reply to comment #4)
> I'm just concerned that this might be introducing a bit of function call
> overhead to get the byte order.

Potentially; some profiling is needed before I merge this. At the moment I could even inline it (DBusHeader.data is public) but for Bug #38288 I'd ideally like to replace DBusHeader.data with something a bit more clever.

Benchmarked on my laptop in a "release" build of dbus (./configure with no special options, equivalent to --disable-asserts --disable-verbose CFLAGS="-g -O2").

Command used:

DBUS_TEST_DAEMON=build-rel/bus/dbus-daemon DBUS_TEST_DATA=test/data ./build-rel/test/test-dbus-daemon --verbose -m perf

and look for the message that mentions "MAXPERF". Fewer seconds => better.

This test is rather artificial (it just shovels 100k identical messages through a dbus-daemon), but for what it's worth, any extra cost from not caching the byte order seems to be lost in the noise (it was less than 1%, and the variation between runs is bigger than that).

# current master: 3 runs, results discarded (cold cache?), mean was > 8 seconds

# redundant-byte-order
(MAXPERF:100000 messages / 8.105744 seconds)
(MAXPERF:100000 messages / 7.782135 seconds)
(MAXPERF:100000 messages / 7.704076 seconds)
mean: 7.86s

# _dbus_header_get_byte_order moved to header and made "static inline"
(MAXPERF:100000 messages / 7.781046 seconds)
(MAXPERF:100000 messages / 7.915617 seconds)
(MAXPERF:100000 messages / 7.850029 seconds)
mean: 7.85s

# excessive inlining (as above, plus replacing DBusString use with direct
# access to the first byte of the DBusString's underlying buffer)
(MAXPERF:100000 messages / 7.788975 seconds)
(MAXPERF:100000 messages / 7.837500 seconds)
(MAXPERF:100000 messages / 7.957807 seconds)
mean: 7.86s

# current master again
(MAXPERF:100000 messages / 7.905754 seconds)
(MAXPERF:100000 messages / 7.706257 seconds)
(MAXPERF:100000 messages / 7.706257 seconds)
mean: 7.79s

(In reply to comment #4)
> I'm just concerned that this might be introducing a bit of function call
> overhead to get the byte order.

(In reply to comment #7)
> any extra cost from not caching the
> byte order seems to be lost in the noise (it was less than 1%, and the
> variation between runs is bigger than that).

Committed to master; we can consider re-optimizing this (inlining the accessor?) after fixing Bug #38288, Bug #38289.