Standardize facelift dbus communication

[idleroamer]

[WIP] Standardize facelift dbus communication
Change-Id: Ifd09ce0e4b16a6a060d2a2634adc614f56b3a139

[jacky309]

We went away from using the "native" DBus serialization a while ago, for 2 reasons:
- Performance
- We have reached the maximum number of arguments which can be written in a message.

AFAIU, your commit reverts that change, which is a bad idea.

[bitmouse]

We went away from using the "native" DBus serialization a while ago, for 2 reasons:

• Performance
• We have reached the maximum number of arguments which can be written in a message.

AFAIU, your commit reverts that change, which is a bad idea.

1. This commit is PoC for further discussion and testing.
2. If what you say is true, then we should probably drop DBus and use other standard. Either we should use common standards or different tool/protocol.
3. I don't really believe in performance argument. Benchmarks to be done (I was not aware of this argument up to now).
4. If we have to drop native DBus then we should probably use some existing tool that allows us to communicate with other languages/systems, e.g. flatbuffers. But then we should probably use sockets directly.

[idleroamer]

I could only find the following limitations:

• The maximum length of a signature is 255.
• The maximum depth of container type nesting is 32 array type codes and 32 open parentheses. This implies that the maximum total depth of recursion is 64, for an "array of array of array of ... struct of struct of struct of ..." where there are 32 array and 32 struct.
  Are you referring to those limitations or others? Because IMHO those are generous limits!

[bitmouse]

• We have reached the maximum number of arguments which can be written in a message.

as noticed this was most probably due to incorrect serialization of lists of structures, so if done properly it should not be the case

[jacky309]

We went away from using the "native" DBus serialization a while ago, for 2 reasons:

• Performance
• We have reached the maximum number of arguments which can be written in a message.

AFAIU, your commit reverts that change, which is a bad idea.

1. This commit is PoC for further discussion and testing.
2. If what you say is true, then we should probably drop DBus and use other standard. Either we should use common standards or different tool/protocol.

Only the serialization part of DBus is not used in a classical manner. We need an IPC to deliver our messages, which is where DBus useful for us.

3. I don't really believe in performance argument. Benchmarks to be done (I was not aware of this argument up to now).

Feel free to write a benchmark if you really need that in order to be convinced.

4. If we have to drop native DBus then we should probably use some existing tool that allows us to communicate with other languages/systems, e.g. flatbuffers. But then we should probably use sockets directly.

flatbuffers is a serialization library, it provides no IPC.

[jacky309]

• We have reached the maximum number of arguments which can be written in a message.

as noticed this was most probably due to incorrect serialization of lists of structures, so if done properly it should not be the case

[jacky309]

• We have reached the maximum number of arguments which can be written in a message.

as noticed this was most probably due to incorrect serialization of lists of structures, so if done properly it should not be the case

The problem is that Qt is missing an API which would be needed to serialize structures "properly". I have spent a lot of time trying to find an acceptable workaround. Maybe that API is available in a more recent version of Qt though.
FYI, the commit which introduced the "serialization as byte array" is dc9e4ca.

[idleroamer]

IDK about the time of facelift initial implementation but as Qt5.12 the API covers all requirements.
My initial benchmarking doesn't show any regression (as I suspected the overhead in dbus is communication not the property marshal/unmarshalling).

[jacky309]

What kind of data did you use in your benchmark ? The overhead introduced by the serialisation is not significant if only a small amount of data is being transferred, so please adjust your benchmark to include large data such as long lists. Also, the registration of all those types in QDBus type system probably has a signification performance cost at startup.
Your change introduces a dependency to DBus into the "local" and "common" IPC components, which is not acceptable.
I did not dig into the details of your changes but I would not be surprised if the support for sub-interfaces over IPC was lost (especially sub-interfaces in structures).
Also, Facelift enables the change of multiple properties at once in an atomic way, which can be important if the values of those properties are related and should be consistent when being exposed. When exposed over IPC, the consistency should be kept. I suspect your change to break that consistency.

If you resolve the merge conflict, we can see if some tests fail, but keep in mind that our test coverage is still incomplete.

[idleroamer]

We are doing further benchmarking and will include the QDbus type registerations as well.
I tried not to break the "interfaces", even though the whole encapsulation is disaster, especially there are many inconsistency between list, map of "interfaces" and stand-alone ones. We need to follow up on that in some other effort.
The simultaneous change of multiple properties doesn't make sense to me at all. If a set of properties are dependent and represent state of any object, following Object-Oriented design, they should be packed into a struct, beside nobody expects this as well and it is not dbus conformant.

[idleroamer]

@jacky309 Can you please let me know what is still blocking the merge?

Check the comments I made during the last days.

All of them are addressed.

[jacky309]

@jacky309 Can you please let me know what is still blocking the merge?

Check the comments I made during the last days.

All of them are addressed.

Rejecting is not addressing.

[idleroamer]

@jacky309 Can you please let me know what is still blocking the merge?

Check the comments I made during the last days.

All of them are addressed.

Rejecting is not addressing.

Either you are having a dialogue about the review points which involves you delivering arguments against the latest comments otherwise they are consider resolved. Or you consider this a one-way street PR which is not a open source culture and there is no point for either of us continue on this.

[idleroamer]

Please remove "request a change", no points are open.

[jacky309]

You mentioned a noticeable drop in performance. Could you please provide some details ?

Most people simply do not care about having a "native" DBus serialization but they do care about having an IPC layer which performs well with their use cases. I do not think it is reasonable to ask anyone to work around a performance drop introduced by a feature which they do not need.
I do think the possibility of having a native DBus support is very good though, but you should make sure that people who do not need do not have to pay the price for it. My suggestion is to introduce an annotation at the interface level, which can be used to specify which kind of serialization should be used.

[idleroamer]

I took a very extreme example where a IPC interface has methods with structs over hundreds of fields, making RPC calls hundreds of times in a row, to see how libdbus acts.
In real use-cases with moderate size structs and reasonable num of IPC calls, the overhead is not even measurable.

So "Nobody" won't feel any difference! We have a huge project with all possible use-cases and observed no difference.

Anyhow if for any reason such unrealistic use-case (hunders of fields in struct) is needed, "toByteArrayOverDBus" annotation for structs will relieve one from signature validation in libdbus and remove any possible overhead.

The annotation should be on struct level as they could cause overhead with their size not the interface.

[jacky309]

I took a very extreme example where a IPC interface has methods with structs over hundreds of fields, making RPC calls hundreds of times in a row, to see how libdbus acts.
In real use-cases with moderate size structs and reasonable num of IPC calls, the overhead is not even measurable.

So "Nobody" won't feel any difference! We have a huge project with all possible use-cases and observed no difference.

Unless this has changed recently, your project is mostly single-process, so I understand that the difference can hardly be noticed... There was a noticeable performance improvement when switching to the QByteArray serialization when multi-process was used.

Anyhow if for any reason such unrealistic use-case (hunders of fields in struct) is needed, "toByteArrayOverDBus" annotation for structs will relieve one from signature validation in libdbus and remove any possible overhead.

Lists of reasonably large structs should be supported with reasonable performance. That kind of list can easily contain up to hundred fields.

The annotation should be on struct level as they could cause overhead with their size not the interface.

Either an interface requires the interoperability feature and it needs to be carefully designed to perform well over IPC, or it does not use need interoperability (which is probably the most common situation) and it should benefit from an optimal serialization performance and not have any limitation in terms of signature length. The decision is based on how the interface is used, so I strongly believe that the annotation should belong to the interface definition.
Additional reasons:
- Data types can be used in multiple interfaces. Enabling the "toByteArrayOverDBus" flag on a type could affect some interfaces which need the interoperability features, making them unusable (or partly usable).
- Whether the signature length limit is reached or not depends on what types are packed together in a single DBus message, which means it depends on the interface definition. It could be that an interface using a large struct does not reach the limit, while another interface using the same struct hits the limit because it is used in combination with other large types (think about multiple method arguments, or multiple properties).

[idleroamer]

Obviously the Multi-process part is in focus. My benchmarks don't show any measurable performance change in realistic cases, we are talking about less than fractions of ms per calls.
I can't believe the improvement you observed by switching to QByteArray was relevant to scope of this change.

Unless this has changed recently, your project is mostly single-process, so I understand that the difference can hardly be noticed... There was a noticeable performance improvement when switching to the QByteArray serialization when multi-process was used.

Lists/Maps doesn't increase the libdbus overhead, signature is increased only by "a()".
Obviously more data is transferred in case of lists but the cost is constant in both current implementation and current PR.
i.e. for each additional entry in the list it takes the same exact amount of time longer.

Lists of reasonably large structs should be supported with reasonable performance. That kind of list can easily contain up to hundred fields.

It is not a feature to be conformant with the DBus rules, it is a bug not to be.
The question is what is the default behavior rather than where annotation should go.
My analysis shows the serialization to ByteArray over DBus has very very small advantage in real use-cases so that it should be used for very corner cases (if at all required). We should encourage developers to design the interfaces more modular rather than using huge structs.
I see pros/cons in placing annotation on struct or on interface. But IMO no exclusivity, so we could introduce an annotation on interface level if needed.

Either an interface requires the interoperability feature and it needs to be carefully designed to perform well over IPC, or it does not use need interoperability (which is probably the most common situation) and it should benefit from an optimal serialization performance and not have any limitation in terms of signature length. The decision is based on how the interface is used, so I strongly believe that the annotation should belong to the interface definition.
Additional reasons:

• Data types can be used in multiple interfaces. Enabling the "toByteArrayOverDBus" flag on a type could affect some interfaces which need the interoperability features, making them unusable (or partly usable).

No it is not how it works, the limit is per method argument not the accumulation of them.

• Whether the signature length limit is reached or not depends on what types are packed together in a single DBus message, which means it depends on the interface definition. It could be that an interface using a large struct does not reach the limit, while another interface using the same struct hits the limit because it is used in combination with other large types (think about multiple method arguments, or multiple properties).