Re: [PATCH 2/5] dbus: Coalesce PropertiesChanged signals

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]

Hi Andrew,

On 02/17/2016 09:20 PM, Andrew Zaborowski wrote:
> When multiple properties are changed in one main loop cycle emit only
> one signal per interface with all the changes.
> ---
>   ell/dbus-private.h |   3 +-
>   ell/dbus-service.c | 256 ++++++++++++++++++++++++++++++++++++-----------------
>   2 files changed, 177 insertions(+), 82 deletions(-)
>

<snip>

> @@ -991,76 +1132,25 @@ bool _dbus_object_tree_property_changed(struct l_dbus *dbus,
>   	if (!property)
>   		return false;
>
> -	signature = property->metainfo + strlen(property->metainfo) + 1;
> -
> -	if (instance->interface->handle_old_style_properties) {
> -		signal = l_dbus_message_new_signal(dbus, path,
> -							interface_name,
> -							"PropertyChanged");
> +	rec = l_queue_find(tree->property_changes,
> +				match_property_changes_instance, instance);
>
> -		builder = l_dbus_message_builder_new(signal);
> -
> -		l_dbus_message_builder_append_basic(builder, 's',
> -							property_name);
> -		l_dbus_message_builder_enter_variant(builder, signature);

This looks a bit suspicious.  Should we be also comparing the object 
pointer, not just the instance pointer here?

> +	if (rec) {
> +		if (l_queue_find(rec->properties, match_pointer, property))
> +			return true;
> +	} else {
> +		rec = l_new(struct property_changes, 1);
> +		rec->path = l_strdup(path);
> +		rec->object = object;
> +		rec->instance = instance;
> +		rec->properties = l_queue_new();
>
> -		if (variant) {
> -			memcpy(&value, variant, sizeof(value));
> -			r = l_dbus_message_builder_append_from_iter(builder,
> -								&value);
> -		} else {
> -			r = property->getter(dbus, signal, builder,
> -						instance->user_data);
> -		}
> -
> -		if (r) {
> -			l_dbus_message_builder_leave_variant(builder);
> -
> -			l_dbus_message_builder_finalize(builder);
> -			l_dbus_send(dbus, signal);
> -		}
> -
> -		l_dbus_message_builder_destroy(builder);
> +		l_queue_push_tail(tree->property_changes, rec);
>   	}
>
> -	if (l_queue_find(object->instances, match_interface_instance,
> -				DBUS_INTERFACE_PROPERTIES)) {
> -		signal = l_dbus_message_new_signal(dbus, path,
> -						DBUS_INTERFACE_PROPERTIES,
> -						"PropertiesChanged");
> -
> -		builder = l_dbus_message_builder_new(signal);
> -
> -		l_dbus_message_builder_append_basic(builder, 's',
> -							interface_name);
> -		l_dbus_message_builder_enter_array(builder, "{sv}");
> -		l_dbus_message_builder_enter_dict(builder, "sv");
> -		l_dbus_message_builder_append_basic(builder, 's',
> -							property_name);
> -		l_dbus_message_builder_enter_variant(builder, signature);
> +	l_queue_push_tail(rec->properties, property);
>
> -		if (variant) {
> -			memcpy(&value, variant, sizeof(value));
> -			r = l_dbus_message_builder_append_from_iter(builder,
> -								&value);
> -		} else {
> -			r = property->getter(dbus, signal, builder,
> -						instance->user_data);
> -		}
> -
> -		if (r) {
> -			l_dbus_message_builder_leave_variant(builder);
> -			l_dbus_message_builder_leave_dict(builder);
> -			l_dbus_message_builder_leave_array(builder);
> -			l_dbus_message_builder_enter_array(builder, "s");
> -			l_dbus_message_builder_leave_array(builder);
> -
> -			l_dbus_message_builder_finalize(builder);
> -			l_dbus_send(dbus, signal);
> -		}
> -
> -		l_dbus_message_builder_destroy(builder);
> -	}
> +	schedule_emit_signals(dbus);
>
>   	return true;
>   }

Regards,
-Denis