Bug 38288 - speculative generality in message (de)marshalling increases complexity
Summary: speculative generality in message (de)marshalling increases complexity
Status: REOPENED
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
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-14 03:39 UTC by Simon McVittie
Modified: 2017-05-16 16:00 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
work-in-progress patch which adds DBusWriteVector (16.76 KB, patch)
2011-06-14 03:58 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-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)


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.