Re: [Qemu-devel] Ping: [PATCH] qobject: object_property_add() performance improvement
On Thu, 04 Jun 2015 10:35:17 +0300
Pavel Fedin wrote:
> Hello Luiz! Have you missed this ?
Well, this is a QOM patch. QOM also has an object abstraction, but
it's not the same as QObject.
I'm CC'ing the maintainer.
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>
>
> > -Original Message-
> > From: qemu-devel-bounces+p.fedin=samsung@nongnu.org [mailto:qemu-devel-
> > bounces+p.fedin=samsung@nongnu.org] On Behalf Of Pavel Fedin
> > Sent: Thursday, May 28, 2015 11:42 AM
> > To: qemu-devel@nongnu.org
> > Cc: 'Luiz Capitulino'
> > Subject: [Qemu-devel] [PATCH] qobject: object_property_add() performance
> > improvement
> >
> > The function originally behaves very badly when adding properties with
> > "[*]" suffix.
> > Nomnally these are used for numbering IRQ pins. In order to find the
> > correct starting
> > number the function started from zero and checked for duplicates. This took
> > incredibly
> > long time with large number of CPUs because number of IRQ pins on some
> > architectures
> (like
> > ARM GICv3) gets multiplied by number of CPUs.
> > The solution is to add one more property which caches last used index so
> > that
> duplication
> > check is not repeated thousands of times. Every time an array is expanded
> > the index is
> > picked up from this cache. Cache property is a uint32_t and has the
> > original name of the
> > array ('name[*]') for simplicity.
> > Some more improvements:
> > - Call object_property_add_single() instead of recursing into itself -
> > keeps off
> memcmp()
> > check every time when its result is already known.
> > - Allocate name_no_array only once and not every time for every property
> > (there can be
> > thousands of them)
> > The modification decreases qemu startup time with 32 ARMv8 CPUs by a
> > factor of 2 (~10
> sec
> > vs ~20 sec).
> >
> > Signed-off-by: Pavel Fedin
> > ---
> > qom/object.c | 89
> > ++--
> > 1 file changed, 62 insertions(+), 27 deletions(-)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index b8dff43..72480bc 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -10,6 +10,8 @@
> > * See the COPYING file in the top-level directory.
> > */
> >
> > +#include
> > +
> > #include "qom/object.h"
> > #include "qemu-common.h"
> > #include "qapi/visitor.h"
> > @@ -721,35 +723,14 @@ void object_unref(Object *obj)
> > }
> > }
> >
> > -ObjectProperty *
> > -object_property_add(Object *obj, const char *name, const char *type,
> > -ObjectPropertyAccessor *get,
> > -ObjectPropertyAccessor *set,
> > -ObjectPropertyRelease *release,
> > -void *opaque, Error **errp)
> > +static ObjectProperty *
> > +object_property_add_single(Object *obj, const char *name, const char *type,
> > + ObjectPropertyAccessor *get,
> > + ObjectPropertyAccessor *set,
> > + ObjectPropertyRelease *release,
> > + void *opaque, Error **errp)
> > {
> > ObjectProperty *prop;
> > -size_t name_len = strlen(name);
> > -
> > -if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
> > -int i;
> > -ObjectProperty *ret;
> > -char *name_no_array = g_strdup(name);
> > -
> > -name_no_array[name_len - 3] = '\0';
> > -for (i = 0; ; ++i) {
> > -char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
> > -
> > -ret = object_property_add(obj, full_name, type, get, set,
> > - release, opaque, NULL);
> > -g_free(full_name);
> > -if (ret) {
> > -break;
> > -}
> > -}
> > -g_free(name_no_array);
> > -return ret;
> > -}
> >
> > QTAILQ_FOREACH(prop, &obj->properties, node) {
> > if (strcmp(prop->name, name) == 0) {
> > @@ -774,6 +755,60 @@ object_property_add(Object *obj, const char *name,
> > const char
> *type,
> > return prop;
> > }
> >
> > +static void property_get_uint32_ptr(Object *obj, Visitor *v,
> > + void *opaque, const char *name,
> > + Error **errp);
> > +
> > +ObjectProperty *
> > +object_property_add(Object *obj, const char *name, const char *type,
> > +ObjectPropertyAccessor *get,
> > +ObjectPropertyAccessor *set,
> > +ObjectPropertyRelease *release,
> > +void *opaque, Error **errp)
> > +{
> > +size_t name_len = strlen(name);
> > +
> > +if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
> > +int i;
> > +ObjectProperty *ret, *count;
> > +/* 10 characters for maximum possible integer number */
> > +char *name_no_array = g_m
Re: [Qemu-devel] Ping: [PATCH] qobject: object_property_add() performance improvement
Hello! > >> + void *v = g_malloc0(sizeof(uint32_t)); > >> + > > Can you allocate name_no_array here along with the count in a small > struct to save on some allocs and memcpys? How can i? Here i allocate persistent storage space for the property, which is kept along. And name_no_array is a temporary buffer which is freed after usage because property names are copied when respective properties are added. > >> +count = object_property_add_single(obj, name, "uint32", > >> + property_get_uint32_ptr, > >> NULL, > > Do you need to register the getter or can you make it completely > opaque instead? I don't need getter, but i decided to register it just in case, because i suggest that it could be useful for example for manual object inspection using monitor. Does monitor have this capability? > Alternatively can you register the setter and use > set/get instead of going hands on with the property opaque pointer? I wanted to do this but i remember having something like object_property_get_int() function but missing object_property_set_int() counterpart (sorry i may have forgotten the exact names) because apparently settable properties start only from qdev class. I decided not to write too much code for a single use case. It's even slightly faster to use a pointer. And by design this property is supposed to be read-only, because anyway setting it from outside isn't a good idea. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
Re: [Qemu-devel] Ping: [PATCH] qobject: object_property_add() performance improvement
On Thu, Jun 4, 2015 at 12:35 AM, Pavel Fedin wrote:
> Hello Luiz! Have you missed this ?
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>
>
>> -Original Message-
>> From: qemu-devel-bounces+p.fedin=samsung@nongnu.org [mailto:qemu-devel-
>> bounces+p.fedin=samsung@nongnu.org] On Behalf Of Pavel Fedin
>> Sent: Thursday, May 28, 2015 11:42 AM
>> To: qemu-devel@nongnu.org
>> Cc: 'Luiz Capitulino'
>> Subject: [Qemu-devel] [PATCH] qobject: object_property_add() performance
>> improvement
>>
>> The function originally behaves very badly when adding properties with
>> "[*]" suffix.
>> Nomnally these are used for numbering IRQ pins. In order to find the correct
>> starting
"normally"
>> number the function started from zero and checked for duplicates. This took
>> incredibly
>> long time with large number of CPUs because number of IRQ pins on some
>> architectures
> (like
>> ARM GICv3) gets multiplied by number of CPUs.
>> The solution is to add one more property which caches last used index so
>> that
> duplication
>> check is not repeated thousands of times. Every time an array is expanded
>> the index is
>> picked up from this cache. Cache property is a uint32_t and has the original
>> name of the
>> array ('name[*]') for simplicity.
>> Some more improvements:
>> - Call object_property_add_single() instead of recursing into itself - keeps
>> off
> memcmp()
>> check every time when its result is already known.
>> - Allocate name_no_array only once and not every time for every property
>> (there can be
>> thousands of them)
>> The modification decreases qemu startup time with 32 ARMv8 CPUs by a factor
>> of 2 (~10
> sec
>> vs ~20 sec).
>>
>> Signed-off-by: Pavel Fedin
>> ---
>> qom/object.c | 89
>> ++--
>> 1 file changed, 62 insertions(+), 27 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index b8dff43..72480bc 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -10,6 +10,8 @@
>> * See the COPYING file in the top-level directory.
>> */
>>
>> +#include
>> +
>> #include "qom/object.h"
>> #include "qemu-common.h"
>> #include "qapi/visitor.h"
>> @@ -721,35 +723,14 @@ void object_unref(Object *obj)
>> }
>> }
>>
>> -ObjectProperty *
>> -object_property_add(Object *obj, const char *name, const char *type,
>> -ObjectPropertyAccessor *get,
>> -ObjectPropertyAccessor *set,
>> -ObjectPropertyRelease *release,
>> -void *opaque, Error **errp)
>> +static ObjectProperty *
>> +object_property_add_single(Object *obj, const char *name, const char *type,
>> + ObjectPropertyAccessor *get,
>> + ObjectPropertyAccessor *set,
>> + ObjectPropertyRelease *release,
>> + void *opaque, Error **errp)
>> {
>> ObjectProperty *prop;
>> -size_t name_len = strlen(name);
>> -
>> -if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
>> -int i;
>> -ObjectProperty *ret;
>> -char *name_no_array = g_strdup(name);
>> -
>> -name_no_array[name_len - 3] = '\0';
>> -for (i = 0; ; ++i) {
>> -char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
>> -
>> -ret = object_property_add(obj, full_name, type, get, set,
>> - release, opaque, NULL);
>> -g_free(full_name);
>> -if (ret) {
>> -break;
>> -}
>> -}
>> -g_free(name_no_array);
>> -return ret;
>> -}
>>
>> QTAILQ_FOREACH(prop, &obj->properties, node) {
>> if (strcmp(prop->name, name) == 0) {
>> @@ -774,6 +755,60 @@ object_property_add(Object *obj, const char *name,
>> const char
> *type,
>> return prop;
>> }
>>
>> +static void property_get_uint32_ptr(Object *obj, Visitor *v,
>> + void *opaque, const char *name,
>> + Error **errp);
>> +
>> +ObjectProperty *
>> +object_property_add(Object *obj, const char *name, const char *type,
>> +ObjectPropertyAccessor *get,
>> +ObjectPropertyAccessor *set,
>> +ObjectPropertyRelease *release,
>> +void *opaque, Error **errp)
>> +{
>> +size_t name_len = strlen(name);
>> +
>> +if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
Rather than if this large block, negate the condition and use
object_property_add_single to short return.
>> +int i;
>> +ObjectProperty *ret, *count;
>> +/* 10 characters for maximum possible integer number */
>> +char *name_no_array = g_malloc(name_len + 10);
>> +
>> +count = object_property_find(obj, name, NULL);
>> + if (count == NULL) {
Indentation doesn't lo
[Qemu-devel] Ping: [PATCH] qobject: object_property_add() performance improvement
Hello Luiz! Have you missed this ?
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
> -Original Message-
> From: qemu-devel-bounces+p.fedin=samsung@nongnu.org [mailto:qemu-devel-
> bounces+p.fedin=samsung@nongnu.org] On Behalf Of Pavel Fedin
> Sent: Thursday, May 28, 2015 11:42 AM
> To: qemu-devel@nongnu.org
> Cc: 'Luiz Capitulino'
> Subject: [Qemu-devel] [PATCH] qobject: object_property_add() performance
> improvement
>
> The function originally behaves very badly when adding properties with "[*]"
> suffix.
> Nomnally these are used for numbering IRQ pins. In order to find the correct
> starting
> number the function started from zero and checked for duplicates. This took
> incredibly
> long time with large number of CPUs because number of IRQ pins on some
> architectures
(like
> ARM GICv3) gets multiplied by number of CPUs.
> The solution is to add one more property which caches last used index so that
duplication
> check is not repeated thousands of times. Every time an array is expanded the
> index is
> picked up from this cache. Cache property is a uint32_t and has the original
> name of the
> array ('name[*]') for simplicity.
> Some more improvements:
> - Call object_property_add_single() instead of recursing into itself - keeps
> off
memcmp()
> check every time when its result is already known.
> - Allocate name_no_array only once and not every time for every property
> (there can be
> thousands of them)
> The modification decreases qemu startup time with 32 ARMv8 CPUs by a factor
> of 2 (~10
sec
> vs ~20 sec).
>
> Signed-off-by: Pavel Fedin
> ---
> qom/object.c | 89
> ++--
> 1 file changed, 62 insertions(+), 27 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index b8dff43..72480bc 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -10,6 +10,8 @@
> * See the COPYING file in the top-level directory.
> */
>
> +#include
> +
> #include "qom/object.h"
> #include "qemu-common.h"
> #include "qapi/visitor.h"
> @@ -721,35 +723,14 @@ void object_unref(Object *obj)
> }
> }
>
> -ObjectProperty *
> -object_property_add(Object *obj, const char *name, const char *type,
> -ObjectPropertyAccessor *get,
> -ObjectPropertyAccessor *set,
> -ObjectPropertyRelease *release,
> -void *opaque, Error **errp)
> +static ObjectProperty *
> +object_property_add_single(Object *obj, const char *name, const char *type,
> + ObjectPropertyAccessor *get,
> + ObjectPropertyAccessor *set,
> + ObjectPropertyRelease *release,
> + void *opaque, Error **errp)
> {
> ObjectProperty *prop;
> -size_t name_len = strlen(name);
> -
> -if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
> -int i;
> -ObjectProperty *ret;
> -char *name_no_array = g_strdup(name);
> -
> -name_no_array[name_len - 3] = '\0';
> -for (i = 0; ; ++i) {
> -char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
> -
> -ret = object_property_add(obj, full_name, type, get, set,
> - release, opaque, NULL);
> -g_free(full_name);
> -if (ret) {
> -break;
> -}
> -}
> -g_free(name_no_array);
> -return ret;
> -}
>
> QTAILQ_FOREACH(prop, &obj->properties, node) {
> if (strcmp(prop->name, name) == 0) {
> @@ -774,6 +755,60 @@ object_property_add(Object *obj, const char *name, const
> char
*type,
> return prop;
> }
>
> +static void property_get_uint32_ptr(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp);
> +
> +ObjectProperty *
> +object_property_add(Object *obj, const char *name, const char *type,
> +ObjectPropertyAccessor *get,
> +ObjectPropertyAccessor *set,
> +ObjectPropertyRelease *release,
> +void *opaque, Error **errp)
> +{
> +size_t name_len = strlen(name);
> +
> +if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
> +int i;
> +ObjectProperty *ret, *count;
> +/* 10 characters for maximum possible integer number */
> +char *name_no_array = g_malloc(name_len + 10);
> +
> +count = object_property_find(obj, name, NULL);
> + if (count == NULL) {
> + void *v = g_malloc0(sizeof(uint32_t));
> +
> +/* This is the same as object_property_add_uint32_ptr(), but:
> + * - Slightly faster and returns pointer
> + * - Will not recurse here so that we can use
> + * raw name with [*] here */
> +count = object_property_add_s
