Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Standardize facelift dbus communication #303

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Standardize facelift dbus communication #303

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
d451419
Standardize facelift argument exchange
Aug 28, 2020
47b4a42
standardize facelift dbus communication
Sep 13, 2020
3c8f9ad
Merge pull request #1 from Pelagicore/master
idleroamer Sep 14, 2020
3d385a1
Merge branch 'master' into feature/standardize_dbus
idleroamer Sep 14, 2020
7fb1d9c
Standardize facelift dbus communication
Sep 14, 2020
698e225
Standardize facelift dbus communication
Sep 16, 2020
2cf600d
standardize_dbus communication
Sep 20, 2020
a3e6151
Fix consistency of the interface in case of multiple property change
Sep 20, 2020
10557c3
Fix review comments
Sep 20, 2020
3ed6fe9
enable defining bus address
Oct 7, 2020
92e7677
fix more review comments
idleroamer Oct 8, 2020
277b7e5
Revert "enable defining bus address"
idleroamer Oct 9, 2020
73c9a06
Fix more review comments
idleroamer Oct 13, 2020
8e122fb
Merge branch 'master' into feature/standardize_dbus
idleroamer Oct 13, 2020
33b996d
Merge branch 'master' of https://github.com/Pelagicore/facelift into …
idleroamer Oct 22, 2020
8e2bd56
Merge pull request #4 from Pelagicore/master
idleroamer Oct 22, 2020
adcce61
Merge branch 'Pelagicore-master-1' into feature/standardize_dbus
idleroamer Oct 22, 2020
b359770
Unify PropertiesChanged routines
idleroamer Nov 1, 2020
1d94e76
Merge branch 'master' into feature/standardize_dbus
idleroamer Nov 1, 2020
1e5ab12
Merge remote-tracking branch 'upstream/master' into feature/standardi…
idleroamer Nov 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions codegen/facelift/facelift-codegen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def requiredIncludeFromType(symbol, suffix):

def insertUniqueType(symbol, unique_types):
type = symbol.type.nested if symbol.type.nested else symbol.type
if type not in (t.name for t in unique_types):
if type.name not in (t.name for t in unique_types):
unique_types.append(type)

def referencedTypes(self):
Expand All @@ -149,21 +149,23 @@ def referencedTypes(self):
insertUniqueType(param, types)
return types

def appendTypeIfStructure(symbol, list):
def appendTypeIfStructureAndUnique(symbol, unique_list):
type = symbol.type.nested if symbol.type.nested else symbol.type
if type.is_struct:
list.append(type)
if type.is_struct and type.name not in (t.name for t in unique_list):
unique_list.append(type)

def referencedStructureTypes(self):
interfaces = []
for property in self.properties:
appendTypeIfStructure(property, interfaces)
appendTypeIfStructureAndUnique(property, interfaces)
for m in self.operations:
for param in m.parameters:
appendTypeIfStructure(param, interfaces)
appendTypeIfStructureAndUnique(param, interfaces)
if m.hasReturnValue:
appendTypeIfStructureAndUnique(m.type, interfaces)
for m in self.signals:
for param in m.parameters:
appendTypeIfStructure(param, interfaces)
appendTypeIfStructureAndUnique(param, interfaces)
return interfaces

def appendTypeIfInterface(symbol, list):
Expand Down Expand Up @@ -211,6 +213,9 @@ def isQMLImplementationEnabled(self):
def isSerializable(self):
return True if self.tags.get('serializable') else generateAll

def toByteArrayOverDBus(self):
return True if self.tags.get('toByteArrayOverDBus') else generateAll

def isQObjectWrapperEnabled(self):
return True if self.tags.get('qml-component') else False

Expand Down Expand Up @@ -309,6 +314,7 @@ def cppMethodArgumentType(self):

setattr(qface.idl.domain.Struct, 'verifyStruct', property(verifyStruct))
setattr(qface.idl.domain.Struct, 'isSerializable', property(isSerializable))
setattr(qface.idl.domain.Struct, 'toByteArrayOverDBus', property(toByteArrayOverDBus))
setattr(qface.idl.domain.Struct, 'isQObjectWrapperEnabled', property(isQObjectWrapperEnabled))
setattr(qface.idl.domain.Struct, 'isQObjectWrapperDeprecated', property(isQObjectWrapperDeprecated))

Expand Down Expand Up @@ -367,6 +373,7 @@ def run_generation(input, output, dependency, libraryName, all):
generateFile(generator, 'module/{{path}}/Module.cpp', 'Module.template.cpp', ctx, libraryName, "")
generateFile(generator, 'ipc/{{path}}/ModuleIPC.h', 'ModuleIPC.template.h', ctx, libraryName, "")
generateFile(generator, 'ipc/{{path}}/ModuleIPC.cpp', 'ModuleIPC.template.cpp', ctx, libraryName, "")

for interface in module.interfaces:
log.debug('process interface %s' % interface)
ctx.update({'interface': interface})
Expand Down
13 changes: 0 additions & 13 deletions codegen/facelift/templates/IPCCommon.template.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,25 +47,12 @@ class {{classExport}} {{interfaceName}}IPCCommon
{{operation.name}},
{% endfor %}
{% for property in interface.properties %}
{% if (not property.readonly) %}
set{{property.name}},
{% endif %}
{% if (property.type.is_model) %}
{{property.name}}, // model
{% endif %}
{% endfor %}
};

enum class SignalID {
invalid = static_cast<int>(facelift::CommonSignalID::firstSpecific),
{% for signal in interface.signals %}
{{signal.name}},
{% endfor %}
{% for property in interface.properties %}
{{property.name}},
{% endfor %}
};

};

{{module.namespaceCppClose}}
17 changes: 9 additions & 8 deletions codegen/facelift/templates/IPCDBusServiceAdapter.template.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include "IPCDBusServiceAdapter.h"
#include "IPCAdapterModelPropertyHandler.h"
#include "DBusManager.h"
#include "FaceliftDBusMarshaller.h"

#include "{{module.fullyQualifiedPath}}/{{interfaceName}}.h"
#include "{{module.fullyQualifiedPath}}/{{interfaceName}}QMLAdapter.h"
Expand All @@ -67,7 +68,6 @@ class {{classExport}} {{className}}: public {{baseClass}}
using ServiceType = {{interfaceName}};
using BaseType = {{baseClass}};
using ThisType = {{className}};
using SignalID = {{interface}}IPCCommon::SignalID;
using MethodID = {{interface}}IPCCommon::MethodID;

{{className}}(QObject* parent = nullptr) :
Expand All @@ -87,7 +87,13 @@ class {{classExport}} {{className}}: public {{baseClass}}

void connectSignals() override;

void serializePropertyValues(OutputIPCMessage& msg, bool isCompleteSnapshot) override;
QVariantMap changedProperties();

QVariantMap marshalProperties() override;

QVariant marshalProperty(const QString& propertyName) override;

void setProperty(const QString& propertyName, const QVariant& value) override;

{% for event in interface.signals %}
void {{event}}(
Expand All @@ -96,7 +102,7 @@ class {{classExport}} {{className}}: public {{baseClass}}
{{ comma() }}{{parameter.interfaceCppType}} {{parameter.name}}
{%- endfor -%} )
{
sendSignal(SignalID::{{event}}
sendSignal("{{event}}"
{%- for parameter in event.parameters -%}
, {{parameter.name}}
{%- endfor -%} );
Expand All @@ -107,14 +113,9 @@ class {{classExport}} {{className}}: public {{baseClass}}
{% for property in interface.properties %}
{% if property.type.is_model %}
::facelift::IPCAdapterModelPropertyHandler<ThisType, {{property.nestedType.interfaceCppType}}> m_{{property.name}}Handler;
{% elif property.type.is_interface %}
QString m_previous{{property.name}}ObjectPath;
{% else %}
{{property.interfaceCppType}} m_previous{{property.name}} {};
{% endif %}
{% if property.type.is_interface %}
InterfacePropertyIPCAdapterHandler<{{property.cppType}}, {{property.cppType}}{{proxyTypeNameSuffix}}> m_{{property.name}};
{% endif %}
{% endfor %}
};

Expand Down
165 changes: 108 additions & 57 deletions codegen/facelift/templates/IPCProxyAdapter.template.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,10 @@
{% set className = interfaceName + proxyTypeNameSuffix %}

#include "{{className}}.h"

{{module.namespaceCppOpen}}

{{className}}::{{className}}(QObject *parent) : BaseType(parent)
{% for property in interface.properties %}
{% if property.type.is_interface %}
, m_{{property.name}}Proxy(*this)
{% endif %}
{% if property.type.is_model %}
, m_{{property.name}}(*this)
{% endif %}
Expand All @@ -55,76 +51,132 @@
{% endif %}
}

void {{className}}::deserializePropertyValues(InputIPCMessage &msg, bool isCompleteSnapshot)
void {{className}}::unmarshalProperties(const QVariantMap& values)
idleroamer marked this conversation as resolved.
Show resolved Hide resolved
{
{% for property in interface.properties %}
{% if property.type.is_interface %}
bool emit_{{property.name}}ChangeSignal = false;
QString {{property.name}}_objectPath;
if (deserializeOptionalValue(msg, {{property.name}}_objectPath, isCompleteSnapshot))
{
m_{{property.name}}Proxy.update({{property.name}}_objectPath);
m_{{property.name}} = m_{{property.name}}Proxy.getValue();
emit_{{property.name}}ChangeSignal = true;
QMap<QString, bool> emitChangeSignal;
idleroamer marked this conversation as resolved.
Show resolved Hide resolved
for (const QString &propertyName: values.keys()) {
{% for property in interface.properties %}
if (propertyName == QStringLiteral("{{property.name}}")) {
{% if property.type.is_interface %}
const {{property.interfaceCppType}} previous_{{property.name}}_Value = m_{{property.name}};
m_{{property.name}} = castFromQVariant<{{property.interfaceCppType}}>(values[propertyName]);
emitChangeSignal[QStringLiteral("{{property.name}}")] = ((previous_{{property.name}}_Value != m_{{property.name}}));
{% elif property.type.is_model %}
emitChangeSignal[QStringLiteral("{{property.name}}")] = true;
int {{property.name}}Size = castFromQVariant<int>(values[propertyName]);
m_{{property.name}}.beginResetModel();
m_{{property.name}}.reset({{property.name}}Size, std::bind(&ThisType::{{property.name}}Data, this, std::placeholders::_1));
m_{{property.name}}.endResetModel();
{% else %}
const auto previous_{{property.name}}_Value = m_{{property.name}};
m_{{property.name}} = castFromQVariant<{{property.interfaceCppType}}>(values[propertyName]);
emitChangeSignal[QStringLiteral("{{property.name}}")] = ((previous_{{property.name}}_Value != m_{{property.name}}));
{% endif %}
}
idleroamer marked this conversation as resolved.
Show resolved Hide resolved
{% endfor %}
if (propertyName == QStringLiteral("ready")) {
bool previousIsReady = this->ready();
m_serviceReady = castFromQVariant<bool>(values[propertyName]);
emitChangeSignal[QStringLiteral("ready")] = (previousIsReady != m_serviceReady);
}
}
{% elif property.type.is_model %}
bool emit_{{property.name}}ChangeSignal = false;
if (isCompleteSnapshot) {
int {{property.name}}Size;
deserializeValue(msg, {{property.name}}Size);
m_{{property.name}}.beginResetModel();
m_{{property.name}}.reset({{property.name}}Size, std::bind(&ThisType::{{property.name}}Data, this, std::placeholders::_1));
m_{{property.name}}.endResetModel();
emit_{{property.name}}ChangeSignal = true;
for (const QString &propertyName: emitChangeSignal.keys()) {
{% for property in interface.properties %}
if (propertyName == QStringLiteral("{{property.name}}") && emitChangeSignal[propertyName]) {
emit {{property.name}}Changed();
}
{% endfor %}
if (propertyName == QStringLiteral("ready") && emitChangeSignal[propertyName]) {
emit readyChanged();
}
}
{% else %}
const auto previous_{{property.name}}_Value = m_{{property.name}};
deserializeOptionalValue(msg, m_{{property.name}}, isCompleteSnapshot);
bool emit_{{property.name}}ChangeSignal = isCompleteSnapshot && ((previous_{{property.name}}_Value != m_{{property.name}}));
{% endif %}
{% endfor %}

bool emit_ReadyChangeSignal = deserializeReadyValue(msg, isCompleteSnapshot) && isCompleteSnapshot;

{% for property in interface.properties %}
if (emit_{{property.name}}ChangeSignal)
emit {{property.name}}Changed();
{% endfor %}

if (emit_ReadyChangeSignal)
emit readyChanged();

}

void {{className}}::deserializeSignal(InputIPCMessage &msg)
void {{className}}::handleSignals(InputIPCMessage& msg)
{
SignalID member;
deserializeValue(msg, member);

Q_UNUSED(msg)
{% for event in interface.signals %}
if (member == SignalID::{{event}})
{
if (msg.member() == QStringLiteral("{{event}}")) {
QListIterator<QVariant> argumentsIterator(msg.arguments());
{% for parameter in event.parameters %}
{{parameter.interfaceCppType}} param_{{parameter.name}};
deserializeValue(msg, param_{{parameter.name}});
param_{{parameter.name}} = (argumentsIterator.hasNext() ? castFromQVariant<{{parameter.interfaceCppType}}>(argumentsIterator.next()):{% if not parameter.type.is_interface %}{{parameter.interfaceCppType}}(){% else %}nullptr{% endif %});
{% endfor %}
emit {{event}}(
{%- set comma = joiner(", ") -%}
{%- for parameter in event.parameters -%}
{{ comma() }}param_{{parameter.name}}
{%- endfor -%} );
} else
idleroamer marked this conversation as resolved.
Show resolved Hide resolved
}
{% endfor %}

{% if interface.hasModelProperty %}
this->onModelUpdateEvent(msg);
{% endif %}
}

const QList<QString>& {{className}}::getSignals() const
idleroamer marked this conversation as resolved.
Show resolved Hide resolved
{
static QList<QString> allSignals{
{% for event in interface.signals %}
"{{event}}",
{% endfor %}
{% if interface.hasModelProperty %}
facelift::IPCCommon::MODEL_DATA_CHANGED_MESSAGE_NAME,
facelift::IPCCommon::MODEL_INSERT_MESSAGE_NAME,
facelift::IPCCommon::MODEL_REMOVE_MESSAGE_NAME,
facelift::IPCCommon::MODEL_MOVE_MESSAGE_NAME,
facelift::IPCCommon::MODEL_RESET_MESSAGE_NAME,
{% endif %}
};

return allSignals;
}

{% if interface.hasModelProperty %}
void {{className}}::onModelUpdateEvent(const InputIPCMessage& msg)
{
QListIterator<QVariant> argumentsIterator(msg.arguments());
const QString& modelPropertyName = (argumentsIterator.hasNext() ? castFromQVariant<QString>(argumentsIterator.next()): QString());
{% for property in interface.properties %}
if (member == SignalID::{{property.name}}) {
{% if property.type.is_model %}
if (modelPropertyName == QStringLiteral("{{property.name}}")) {
m_{{property.name}}.handleSignal(msg);
{% else %}
emit {{property.name}}Changed();
}
{% endif %}
} else
{% endfor %}
BaseType::deserializeCommonSignal(static_cast<facelift::CommonSignalID>(member), this);
}
{% endif %}

void {{className}}::unmarshalPropertiesChanged(const QVariantMap& changedProperties)
{
{% if interface.properties %}
for (const QString &propertyName: changedProperties.keys()) {
{% for property in interface.properties %}
if (propertyName == QStringLiteral("{{property.name}}")) {
{% if property.type.is_model %}
int {{property.name}}Size = castFromQVariant<int>(changedProperties[propertyName]);
m_{{property.name}}.beginResetModel();
m_{{property.name}}.reset({{property.name}}Size, std::bind(&ThisType::{{property.name}}Data, this, std::placeholders::_1));
m_{{property.name}}.endResetModel();
{% else %}
m_{{property.name}} = castFromQVariant<{{property.interfaceCppType}}>(changedProperties[propertyName]);
{% endif %}
}
{% endfor %}
}
for (const QString &propertyName: changedProperties.keys()) {
{% for property in interface.properties %}
if (propertyName == QStringLiteral("{{property.name}}")) {
{% if not property.type.is_model %}
emit {{property.name}}Changed();
{% endif %}
}
{% endfor %}
}
{% else %}
Q_UNUSED(changedProperties);
{% endif %}
}

{% for property in interface.properties %}
Expand All @@ -134,7 +186,7 @@ void {{className}}::deserializeSignal(InputIPCMessage &msg)
void {{className}}::set{{property}}({{property.cppMethodArgumentType}} newValue)
{
{% if (not property.type.is_interface) %}
ipc()->sendSetterCall(memberID(MethodID::set{{property.name}}, "set{{property.name}}"), newValue);
ipc()->sendSetterCall("{{property.name}}", newValue);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the construct you broke here is meant to provide a way for that generated code to be usable with both an IPC transport channel where method IDs are strings (such as DBus), and an IPC where method IDs are integer (such as the the current "local" IPC, and the hi-performance IPC which I have been working on). Using integers instead of strings is obviously more efficient.
Your change will make the introduction of a new IPC harder.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please stop the "performance" discussion? now you are discussing about "future possible performance degrades with some not in-place high performance IPC!!!".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The local IPC uses integer IDs, so we are not talking about the future.. The current approach should work with your changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The performance claims needs to be backed up by actuall KPI measurement
There "Local" version AFAIK uses internal Qt signal/slots so we can't have seriously a discussion over difference of treating a signal with integer and a signal with string!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no doubt that sending an integer (which does not even need to be wider than 8 bits) is faster than sending a multi-character string. Also, string comparisons are not free, so the processing of an incoming request is also faster if an integer is used as method ID since no comparison is even needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple benchmark of signal 30 character long being emitted/received 10000 times shows followings:
https://github.com/idleroamer/facelift/tree/feature/benchmark_local_signals

Current implementation
Signal emission/receive duration: 1463 ms

Current PR:
Signal emission/receive duration:1102 ms

A signal name comparison is not a bottleneck.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do those figures tell us that using a string is faster than using an integer ?
The reason why integers can be used is not really because using strings has been identified as a performance hotspot, but rather that it is necessary for IPCs which use integers as messageID. I do not understand why your change has to break that.

Copy link
Author

@idleroamer idleroamer Oct 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, those figures tell us whether we send MessageIDs as strings or as integers is not a performance bottleneck.
It as well tells us performance arguments without benchmarks has no values.

I appreciate your time but I have the feeling you even haven't looked at the PR? the "sendSetterCall" is internal call between IPCProxy and Local/DBusIPCProxyBinder and using integers is simply superfluous

You had performance claims now you are talking again about some IPC which is not even there yet AGAIN.
Please let me know if you personally hold this PR against your views/plan for facelift so I don't take your time anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a misunderstanding here.
How about having a call tomorrow afternoon ? I guess we could use google meet for that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a fine suggestion. Though tomorrow is a little tight in the afternoon, so let me know if tomorrow morning or the Friday's afternoon fits your calendar, otherwise we postpone to next week.

{% else %}
Q_ASSERT(false); // Writable interface properties are unsupported
{% endif %}
Expand Down Expand Up @@ -168,12 +220,11 @@ void {{className}}::{{operation.name}}(
{%- endfor -%} ){% if operation.is_const %} const{% endif %}
{
{% if (operation.hasReturnValue) %}
{{operation.interfaceCppType}} returnValue;
ipc()->sendMethodCallWithReturn(memberID(MethodID::{{operation.name}}, "{{operation.name}}"), returnValue
QList<QVariant> args = ipc()->sendMethodCallWithReturn(memberID(MethodID::{{operation.name}}, "{{operation.name}}")
{%- for parameter in operation.parameters -%}
, {{parameter.name}}
{%- endfor -%} );
return returnValue;
return (!args.isEmpty() ? castFromQVariant<{{operation.interfaceCppType}}>(args.first()):{% if not (operation.type.is_interface) %}{{operation.cppType}}(){% else %}nullptr{% endif %});
{% else %}
ipc()->sendMethodCall(memberID(MethodID::{{operation.name}}, "{{operation.name}}")
{%- for parameter in operation.parameters -%}
Expand Down
Loading