Bug 38288 - speculative generality in message (de)marshalling increases complexity
Summary: speculative generality in message (de)marshalling increases complexity
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: low enhancement
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
Depends on:
Reported: 2011-06-14 03:39 UTC by Simon McVittie
Modified: 2018-10-12 21:09 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:

work-in-progress patch which adds DBusWriteVector (16.76 KB, patch)
2011-06-14 03:58 UTC, Simon McVittie
Details | Splinter Review

Description Simon McVittie 2011-06-14 03:39:14 UTC
DBusTypeReader and DBusTypeWriter have some rather questionable functionality:

* _dbus_type_reader_set_basic and _dbus_type_reader_delete allow a reader
  to be used for writing (!), used to edit header fields

* DBusTypeWriter operates in terms of insertion, not appending, even though
  DBusMessage is append-only; as a result, it needs to be able to cope with
  realigning arbitrary types in arbitrary positions (even within an array!),
  which accounts for a large proportion of the complexity of marshal-recursive

* when editing a message's header, the same very general code is used to
  realign all header fields after the insertion/deletion point, even though
  header fields are all already 8-byte-aligned due to being structs

* even if we didn't realign, inserting or deleting in the middle of the
  header requires at least a memmove()

Here is a concrete proposal to simplify this:

* Generalize dbus_write_*_two to take a (D-Bus abstraction representing a)
  struct iovec[] (I have a work-in-progress branch which adds
  DBusWriteVector, which is basically a struct iovec[] plus a count of
  bytes written so far)

* If the platform is sufficiently rubbish that it doesn't have either
  sendmsg() or writev(), it can use a slow path involving repeated write()
  (Google suggests that on Windows we could use WSASend(), which is
  basically writev() NIH'd)

* Stop viewing the message header as type (yyyyuua(uv)), and start viewing
  it as a 16-byte blob (yyyyuuu) (where the last u is really the length
  field for the a(uv), updated in the same sort of way as the body length),
  followed by some separate (uv) structs

* Store each (uv) as a (string, start, len) tuple, where either:

  - the string is the original header string, edited fields are appended,
    and the fields they replaced are just not included in the
    struct iovec[]

  - the string is either the original header string, or (for edited fields)
    a second string used as scratch space

  - the string is either the original header string, or one extra string
    per edited field

* Write messages into sockets by using scatter-gather I/O, with a struct
  iovec[n + 2] pointing to the 16-byte fixed part, the n header fields,
  and the body

* Read messages into a single DBusString per header as we do now, and
  let all the header-field tuples point into that string

* Delete all the code that requires realigning and be happy
Comment 1 Simon McVittie 2011-06-14 03:39:27 UTC
For bonus points, we could consider reading messages via scatter-gather I/O too, which would mean we never needed to copy or realign them:

* first read is 16 bytes, for the lengths

* subsequent reads are a struct iovec[5] for:
  * the header
  * the padding after the header, if any (just to check that it's zeroed)
  * the body
  * the padding after the body, if any (just to check that it's zeroed)
  * the next 16 bytes, if another message is immediately available

but we'd need to benchmark that to see if it was actually better in practice.
Comment 2 Simon McVittie 2011-06-14 03:58:52 UTC
Created attachment 47950 [details] [review]
work-in-progress patch which adds DBusWriteVector

See dbus-iovec.h in the patch.

This probably doesn't compile or work, but could be a useful starting point.
Comment 3 Simon McVittie 2017-05-16 16:00:55 UTC
(In reply to Simon McVittie from comment #0)
> * Write messages into sockets by using scatter-gather I/O, with a struct
>   iovec[n + 2] pointing to the 16-byte fixed part, the n header fields,
>   and the body

When I mentioned this to Allison she warned me that this is not really how writev/sendmsg is meant to be used, so we might have to back off from actually doing the scatter/gather part.

However, it seems entirely plausible to have a data structure something like this pseudocode:

    char[16] fixed_length_part;
    DBusString header_field_storage;

and do field insertion, replacement and deletion like this:

    iter = beginning of header_field_storage;

    while ((iter = next field in header_field_storage) != end) {
        if (field ID == the one we want to replace or delete) {
            work out length of this field (it is a struct (yv))
            if (it is the last one in the buffer) {
                truncate immediately after the previous field, deleting padding
            else {
                round up to the next 8-byte unit to count the alignment padding
                memmove everything after that point downwards to cover it
                truncate after the segment we moved

    add 0-7 bytes of new alignment
    append new ID, type, value of field as a new struct (yv)
Comment 4 GitLab Migration User 2018-10-12 21:09:06 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/47.

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.