Standardize facelift dbus communication

[idleroamer]

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

[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 dc9e4ca9234cddf7d7c0171d2c5a137551878c37.

[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.

[bitmouse]

Also, the registration of all those types in QDBus type system probably has a signification performance cost at startup.

Could you explain better what you mean?

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).

To be verified. Please ask for checking instead of personal comments in the future.

Facelift enables the change of multiple properties at once in an atomic way,

yeah... atomic... right... I have never seen any kind of mutex, etc. in facelift. Not even saying about std::atomic.

[jacky309]

Also, the registration of all those types in QDBus type system probably has a signification performance cost at startup.

Could you explain better what you mean?

Just look at the amount of additional code for type registrations. The registration template methods typically expands into large portions of binary code.

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).

To be verified. Please ask for checking instead of personal comments in the future.

There is nothing personal in my comment ! I noticed large portions of code being removed/replaced, including the code which is responsible for the registration of sub-interfaces. This stuff can be broken without noticing at first. This needs to be checked, ideally with a new test case, if this feature is not covered yet.

Facelift enables the change of multiple properties at once in an atomic way,

yeah... atomic... right... I have never seen any kind of mutex, etc. in facelift. Not even saying about std::atomic.

[jacky309]

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.

To make sure we are on the same page, let's consider that example:

interface TunerInterface { int currentFrequency; int maximumFrequency; int minimumFrequency; }

Now, please consider the "AM to FM band switching" use case, where those 3 properties are changed. If you first change the currentFrequency and issue the "currentFrequency changed" signal, you possibly expose an inconsistent state to the slots, since the value of the currentFrequency property is possibly outside of the validity range, since the latter has not been changed yet. A correct way to address this issue is to change all property values first, and then start triggering the corresponding change signals. By doing so, no inconsistent state is exposed to any slot. Facelift's IPC layer preserves that consistency, by making a distinction between those 2 events: "property X changed" and "XChanged signal is triggered".

I agree that moving all those "related properties" into a single structure property would be a way to ensure that the consistency is preserved whatever happens, but this is a workaround which has the following problems: - It results in a weird interface definition since the fields are not grouped in a natural or logical way, but rather in such a way that the consistency is insured. - When a single value has changed, multiple additional fields are also sent over IPC, if they belong to the same structure, even if only one field has changed. This can be an issue with large properties (bitmap or long lists).

[bitmouse]

interface TunerInterface { int currentFrequency; int maximumFrequency; int minimumFrequency; }

Now, please consider the "AM to FM band switching" use case, where those 3 properties are changed. If you first change the currentFrequency and issue the "currentFrequency changed" signal, you possibly expose an inconsistent state to the slots, since the value of the currentFrequency property is possibly outside of the validity range, since the latter has not been changed yet. A correct way to address this issue is to change all property values first, and then start triggering the corresponding change signals. By doing so, no inconsistent state is exposed to any slot. Facelift's IPC layer preserves that consistency, by making a distinction between those 2 events: "property X changed" and "XChanged signal is triggered".

I think this is wrong already on the concept level. At first, the interface itself should be stateless, so it should have no properties. At second, these should be grouped into a structure. The biggest benefit of grouping them into the structure is that we start following the object oriented programming paradigm. Even C language provided structures for the inseparable data, so why do we?

I agree that moving all those "related properties" into a single structure property would be a way to ensure that the consistency is preserved whatever happens, but this is a workaround which has the following problems:

• It results in a weird interface definition since the fields are not grouped in a natural or logical way, but rather in such a way that the consistency is insured.

If we have the proper (stateless) interface, it is not a workaround. If the fields are not grouped in the natural or logical way (structured), then this is the project design problem, not the RPC framework that project uses.

• When a single value has changed, multiple additional fields are also sent over IPC, if they belong to the same structure, even if only one field has changed. This can be an issue with large properties (bitmap or long lists).

If this is a problem, I would suggest to refine the design of the "interface" or structures from project side, because something is wrong at this level.

If something is unclear about grouping: imagine that you have to send Point2D. What is the proper way:

interface Point { int x; int y; }

or

interface Point { Point2D getPoint(); void setPoint(Point2D p); }

struct Point2D { int x; int y; }

Even if only single field has changed, this is now completely different point.

[idleroamer]

@jacky309 We were following up on dbus limit and for some huge structure the 255 character limit of the signature could be an issue. Although we can switching to old binary data transfer for those structs when annotated with "@serializable:true", I am not fully sure what is the main purpose of that annonation but since previously all structs were send like it is no backward compatibility issue. Please have a look at: https://github.com/Pelagicore/facelift/pull/303/commits/c1ad3438471f9250ccf8afe5cfe901efd34d540b

[jacky309]

@jacky309 We were following up on dbus limit and for some huge structure the 255 character limit of the signature could be an issue. Although we can switching to old binary data transfer for those structs when annotated with "@serializable:true", I am not fully sure what is the main purpose of that annonation but since previously all structs were send like it is no backward compatibility issue. Please have a look at: c1ad343

That annotation is documented in the annotations page. It adds the serialize/deserialize methods to the structure API, which is completely unrelated to the IPC layer. Reusing that annotation to specify how the structure is serialized over DBus-IPC sounds like a misuse. I would rather introduce a new annotation to enable an interface to be exposed in a "native DBus" way and I would keep the current serialization approach as default. I still do not know exactly what concrete problem you are trying to solve with that pull request, TBH. Maybe you can clarify that ? If the goal is to improve the interoperability issues, I assume that only some specific interfaces are concerned.

[idleroamer]

Well as you put it the issue is interoperability, we would like to use facelift in combination with other dbus frameworks in other languages. With this PR it is possible to use other frameworks as well (godbus). IDK what do you mean by some specific interfaces is only needed to enable interoperability! We can introduce a new annotation should not be hard. But I see the default behaviour otherway around, all communication should be in "native dbus" (since facelift used dbus and intuitively everybody expects that) unless the user defined it otherwise (due to huge structure signature or otherwise).

[jacky309]

interface TunerInterface { int currentFrequency; int maximumFrequency; int minimumFrequency; } Now, please consider the "AM to FM band switching" use case, where those 3 properties are changed. If you first change the currentFrequency and issue the "currentFrequency changed" signal, you possibly expose an inconsistent state to the slots, since the value of the currentFrequency property is possibly outside of the validity range, since the latter has not been changed yet. A correct way to address this issue is to change all property values first, and then start triggering the corresponding change signals. By doing so, no inconsistent state is exposed to any slot. Facelift's IPC layer preserves that consistency, by making a distinction between those 2 events: "property X changed" and "XChanged signal is triggered".

I think this is wrong already on the concept level. .....

Why not sticking to the example which I suggested ? : Again, I have a radio tuner device, which is a stateful device. Its state is made of 2 things:

• a selected frequency
• a selected wave band Now, if I want to use Facelift to expose an interface which enables someone to interact with my device, I would likely come up with the following QFace definition, which provides some additional properties for maximum and minimum frequencies:

enum WaveBand { AM, FM }

interface TunerInterface { int currentFrequency; readonly int maximumFrequency; readonly int minimumFrequency; WaveBand waveband; }

What is fundamentally wrong with that interface ? Why ? What is not object-oriented here, if all the properties belong to an object (the interface) ?

How would your QFace interface look like, for the same tuner, with the same feature set ? It is important that we have a common understanding of what a good interface looks like, to understand what Facelift should provide.

[jacky309]

Well as you put it the issue is interoperability, we would like to use facelift in combination with other dbus frameworks in other languages. With this PR it is possible to use other frameworks as well (godbus). IDK what do you mean by some specific interfaces is only needed to enable interoperability!

I assume that most QFace interfaces defined in a product using facelift do not require the interoperability feature. Typically, only a couple of them would really require the interoperability annotation.

We can introduce a new annotation should not be hard. But I see the default behaviour otherway around, all communication should be in "native dbus" (since facelift used dbus and intuitively everybody expects that) unless the user defined it otherwise (due to huge structure signature or otherwise).

The current implementation does not have the limitations which your change introduces and I assume it is faster and generates a more compact code (to be confirmed). The current implementation can also easily be improved to get multiple consecutive property change signals to be squashed into a single dbus message. AFAIK, your approach does not allow that.

I understand that the interoperability feature is useful and I can appreciate the work you've done, but the price to pay should definitely be considered.

[bitmouse]

Why not sticking to the example which I suggested ? : Again, I have a radio tuner device, which is a stateful device. Its state is made of 2 things:

* a selected frequency

* a selected wave band
  Now, if I want to use Facelift to expose an interface which enables someone to interact with my device, I would likely come up with the following QFace definition, which provides some additional properties for maximum and minimum frequencies:

enum WaveBand { AM, FM }

interface TunerInterface { int currentFrequency; readonly int maximumFrequency; readonly int minimumFrequency; WaveBand waveband; }

What is fundamentally wrong with that interface ? Why ? What is not object-oriented here, if all the properties belong to an object (the interface) ?

As mentioned before it created state of the interface. So this is not right, the proper interface is state agnostic (stateless). That is the first fail of this kind of "interface" - it has properties. You have created remote object instead of interface. However this still would be acceptable if they were organized properly.

How would your QFace interface look like, for the same tuner, with the same feature set ? It is important that we have a common understanding of what a good interface looks like, to understand what Facelift should provide.

Please consider this:

interface TunerInterface { int currentFrequency; BandInfo bandInfo; }

struct BandInfo { readonly int maximumFrequency; readonly int minimumFrequency; WaveBand waveband; }

The currentFrequency can change independently to other properties, but the other three are tightly coupled. If we change bandInfo now, we must probably change currentFrequency, but this is developer responsibility now to do it right, not the, so called, interface (remote object in fact).

But with further improvement I would suggest (BandInfo structure as above).

interface Tuner { // you do not have to tell it is interface in the name. Using Hungarian notation and its modifications is really bad idea int getCurrentFrequency(); void setCurrentFrequency(int freq); // or bool, where bool is a success status BandInfo getBandInfo(); void setWaveBand(WaveBand wb); signal currentFrequencyChanged(); signal bandChanged(); }

This makes the interface design right, interface is stateless. Using polymorphism in this case makes sense. Also it could be generated to other programming languages as an interface.

The current implementation does not have the limitations which your change introduces and I assume it is faster and generates a more compact code (to be confirmed). The current implementation can also easily be improved to get multiple consecutive property change signals to be squashed into a single dbus message. AFAIK, your approach does not allow that.

This change actually removes limitation to facelift framework in DBus usage. Anyway there are other problems like service registry which is in fact useless and most probably can be removed with minimal effort.

[idleroamer]

I will try to summarize all points here:

Purpose of PR:

Facelift is not a standard dbus framework and hence not interoperable due to following reasons.

• Signatures of properties/methods/signals are missing or are completely wrong
• “org.freedesktop.Dbus.Properties interface” is not utilized
• All signals are send via generic “SignalTriggered”
• Methods doesn’t have right signature All the above makes Facelift impossible to combine in form of server/client schema with other dbus tools/frameworks.

Approach: Refactor the ipc layer in following manner

• Utilize QtDBus tools to generate right signature for structs
• Utilize “org.freedesktop.Dbus.Properties interface” as the standard interface for property exchange
• For each signal use the corresponding signal signature not the generic “SignalTriggered”

Concerns about the PR

• DBus limitation: • The struct in struct limitation of 32 by 32. • The maximum length of a signature is 255. Remedy: Annotation: @treatAsByteArray turns the struct into byte array form over the dbus interfaces. Similar to previous treatment of structs.

Default Behavior: Intuitively and obviously this is not the default behavior of struct, the struct needs to follow dbus standard. If developer runs to such limitation in their code-base, they really need to rethink their design. 255 is really long signature (in whole carbon-ui two auto-generated if1qface structs run into this limit which IMHO are badly design as well).

• Atomic PropertiesChanged of multiple properties: Dbus standard allows sending a “PropertiesChanged” with multiple values at once. Event though AFAIK currently facelift doesn’t have such feature in any form in place but the PR does not block future features in this direction.

org.freedesktop.DBus.Properties.PropertiesChanged (STRING interface_name, DICT<STRING,VARIANT> changed_properties, ARRAY<STRING> invalidated_properties);

• Performance regression Preliminary results show no regression in case of 10000 method calls of methods with different range of method signature (input and output struct) from couple of fields to ~200 fields in struct.

Please reachout for .pref files.

[jacky309]

Why not sticking to the example which I suggested ? : Again, I have a radio tuner device, which is a stateful device. Its state is made of 2 things:

* a selected frequency

* a selected wave band
  Now, if I want to use Facelift to expose an interface which enables someone to interact with my device, I would likely come up with the following QFace definition, which provides some additional properties for maximum and minimum frequencies:

enum WaveBand { AM, FM } interface TunerInterface { int currentFrequency; readonly int maximumFrequency; readonly int minimumFrequency; WaveBand waveband; } What is fundamentally wrong with that interface ? Why ? What is not object-oriented here, if all the properties belong to an object (the interface) ?

As mentioned before it created state of the interface. So this is not right, the proper interface is state agnostic (stateless). That is the first fail of this kind of "interface" - it has properties. You have created remote object instead of interface. However this still would be acceptable if they were organized properly.

How would your QFace interface look like, for the same tuner, with the same feature set ? It is important that we have a common understanding of what a good interface looks like, to understand what Facelift should provide.

Please consider this:

interface TunerInterface { int currentFrequency; BandInfo bandInfo; }

struct BandInfo { readonly int maximumFrequency; readonly int minimumFrequency; WaveBand waveband; }

The currentFrequency can change independently to other properties, but the other three are tightly coupled. If we change bandInfo now, we must probably change currentFrequency, but this is developer responsibility now to do it right, not the, so called, interface (remote object in fact).

But with further improvement I would suggest (BandInfo structure as above).

interface Tuner { // you do not have to tell it is interface in the name. Using Hungarian notation and its modifications is really bad idea int getCurrentFrequency(); void setCurrentFrequency(int freq); // or bool, where bool is a success status BandInfo getBandInfo(); void setWaveBand(WaveBand wb); signal currentFrequencyChanged(); signal bandChanged(); }

This makes the interface design right, interface is stateless. Using polymorphism in this case makes sense. Also it could be generated to other programming languages as an interface.

The current implementation does not have the limitations which your change introduces and I assume it is faster and generates a more compact code (to be confirmed). The current implementation can also easily be improved to get multiple consecutive property change signals to be squashed into a single dbus message. AFAIK, your approach does not allow that.

This change actually removes limitation to facelift framework in DBus usage. Anyway there are other problems like service registry which is in fact useless and most probably can be removed with minimal effort.

So you are suggesting that users of Facelift stop using properties when defining their interfaces. That would mean that : - Interfaces are not QML-friendly anymore. QML shines at handling properties (especially property bindings). One of the primary goals of Facelift is to help exposing interfaces to QML. - A major performance penalty is introduced. Property values now have to be requested one by one, by calling the property getters. - Worst of all: by the time the property getters have returned, the values which have been received by the client are not guaranteed to be consistent, since it could be that the server side changed some properties in between. There is no reliable way, at any point time, for the client to know if the received values represent a state which ever existed on the server side.

Your interface is a perfect example of what NOT to do, especially as far as defining interfaces to be used over IPC. Our RPC framework has to ensure that the state replicated on the client side is a state which existed on the server side and to minimize the replication time. We do not want any user code to be required to realize that reliable replication. AFAIK, the only way to replicate the state reliably is to be able to transport multiple property updates together in a single IPC message, which is what facelift currently does, and we should not break that.

By turning formal properties into methods/signals, you are just hiding the real nature of the interface, which is stateful, and you simply introduce new problems.

[idleroamer]

As I pointed out in the last comment, this PR doesn't put any limitation on how many properties can be changed atomically. org.freedesktop.DBus.Properties.PropertiesChanged (STRING interface_name, DICT<STRING,VARIANT> changed_properties, ARRAY invalidated_properties); Which makes the discussion not anymore relevant to the PR.

So please let's discuss stay on the PR, we needs these changes ASAP.

[jacky309]

Added new test : https://github.com/Pelagicore/facelift/pull/305 Could you please check that your change does not break that test ? Could you please also squash your commits ?

[jacky309]

"serializeOverIPC" is not a good name. Structures are always serialized to be sent over IPC... I would rather have an annotation on an interface, rather than on the structure definition.

[idleroamer]

Added new test : #305 Could you please check that your change does not break that test ? Could you please also squash your commits ?

The test passes. I will squash commits for the last step.

[idleroamer]

"serializeOverIPC" is not a good name. Structures are always serialized to be sent over IPC... I would rather have an annotation on an interface, rather than on the structure definition.

switch to "treatAsByteArray". The annotation is about each single struct not the interface itself. So some structs in interface can be send as byte array while others are still in native signature.

[idleroamer]

@jacky309 Please have another look, all QtDBus stuff moved to DBus layer.

[idleroamer]

@jacky309 Please let me know if you have any further comments, I assume the current ones are all addressed.

[idleroamer]

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

[jacky309]

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

Check the comments I made during the last days.