| Summary: | speculative generality in message (de)marshalling increases complexity | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Simon McVittie <smcv> |
| Component: | core | Assignee: | D-Bus Maintainers <dbus> |
| Status: | RESOLVED MOVED | QA Contact: | D-Bus Maintainers <dbus> |
| Severity: | enhancement | ||
| Priority: | low | CC: | bart.cerneels, chengwei.yang.cn, msniko14, smcv |
| Version: | 1.5 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: | work-in-progress patch which adds DBusWriteVector | ||
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. 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. (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) -- 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.
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