[v3] QOM: object_property_add() performance improvement

The function originally behaves very badly when adding properties with "[*]"
suffix. Normally 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 takes 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.
The property is a uint32_t and has the original name of the array ('name[*]')
for simplicity. It has getter function in order to allow to inspect it from
within monitor.

The modification decreases qemu startup time with 32 CPUs by a factor of 2
(~10 sec vs ~20 sec).

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 Changes since v2:
- Fixed line spacing
- 'i' is now uintptr_t, got rid of double-casting from void *. String space is enlarged to
20 characters in order to accommodate maximum possible 64-bit value.
---
 qom/object.c | 93 ++++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 66 insertions(+), 27 deletions(-)

Ping!

Regards,
Peter

On Tue, Jun 16, 2015 at 12:50 AM, Pavel Fedin <p.fedin@samsung.com> wrote:
> The function originally behaves very badly when adding properties with "[*]"
> suffix. Normally 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 takes 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.
> The property is a uint32_t and has the original name of the array ('name[*]')
> for simplicity. It has getter function in order to allow to inspect it from
> within monitor.
>
> The modification decreases qemu startup time with 32 CPUs by a factor of 2
> (~10 sec vs ~20 sec).
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  Changes since v2:
> - Fixed line spacing
> - 'i' is now uintptr_t, got rid of double-casting from void *. String space is enlarged to
> 20 characters in order to accommodate maximum possible 64-bit value.
> ---
>  qom/object.c | 93 ++++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 66 insertions(+), 27 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 96abd34..e8b94fb 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -10,6 +10,8 @@
>   * See the COPYING file in the top-level directory.
>   */
>
> +#include <glib/gprintf.h>
> +
>  #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,64 @@ object_property_add(Object *obj, const char *name, const char *type,
>      return prop;
>  }
>
> +static void property_get_uint32_opaque(Object *obj, Visitor *v,
> +                                   void *opaque, const char *name,
> +                                   Error **errp)
> +{
> +    uint32_t value = (uintptr_t)opaque;
> +    visit_type_uint32(v, &value, name, 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);
> +    char *name_no_array;
> +    ObjectProperty *ret, *count;
> +    uintptr_t i;
> +
> +    if (name_len < 3 || memcmp(&name[name_len - 3], "[*]", 4)) {
> +        return object_property_add_single(obj, name, type,
> +                                          get, set, release, opaque, errp);
> +    }
> +
> +    /* 20 characters for maximum possible uintptr_t (64-bit) */
> +    name_no_array = g_malloc(name_len + 20);
> +
> +    count = object_property_find(obj, name, NULL);
> +    if (count == NULL) {
> +        /* This is very similar to object_property_add_uint32_ptr(), but:
> +         * - Returns pointer
> +         * - Will not recurse here so that we can use raw name with [*]
> +         * - Allows us to use 'opaque' pointer itself as a storage, because
> +         *   we want to store only a single integer which should not be
> +         *   modified from outside.
> +         */
> +        count = object_property_add_single(obj, name, "uint32",
> +                                           property_get_uint32_opaque, NULL,
> +                                           NULL, NULL, &error_abort);
> +    }
> +
> +    name_len -= 3;
> +    memcpy(name_no_array, name, name_len);
> +    i = (uintptr_t)count->opaque;
> +
> +    do {
> +        g_sprintf(&name_no_array[name_len], "[%zu]", i++);
> +
> +        ret = object_property_add_single(obj, name_no_array, type, get, set,
> +                                         release, opaque, NULL);
> +    } while (!ret);
> +
> +    count->opaque = (void *)i;
> +    g_free(name_no_array);
> +    return ret;
> +}
> +
>  ObjectProperty *object_property_find(Object *obj, const char *name,
>                                       Error **errp)
>  {
> --
> 1.9.5.msysgit.0
>
>
>

I am here, was checking submission statuses and was going to PING too...
 Andreas???!

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

> -----Original Message-----
> From: qemu-devel-bounces+p.fedin=samsung.com@nongnu.org [mailto:qemu-devel-
> bounces+p.fedin=samsung.com@nongnu.org] On Behalf Of Peter Crosthwaite
> Sent: Saturday, June 27, 2015 6:28 AM
> To: Pavel Fedin
> Cc: QEMU Developers; Andreas Färber
> Subject: Re: [Qemu-devel] [PATCH v3] QOM: object_property_add() performance improvement
> 
> Ping!
> 
> Regards,
> Peter
> 
> On Tue, Jun 16, 2015 at 12:50 AM, Pavel Fedin <p.fedin@samsung.com> wrote:
> > The function originally behaves very badly when adding properties with "[*]"
> > suffix. Normally 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 takes 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.
> > The property is a uint32_t and has the original name of the array ('name[*]')
> > for simplicity. It has getter function in order to allow to inspect it from
> > within monitor.
> >
> > The modification decreases qemu startup time with 32 CPUs by a factor of 2
> > (~10 sec vs ~20 sec).
> >
> > Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> > Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > ---
> >  Changes since v2:
> > - Fixed line spacing
> > - 'i' is now uintptr_t, got rid of double-casting from void *. String space is enlarged to
> > 20 characters in order to accommodate maximum possible 64-bit value.
> > ---
> >  qom/object.c | 93 ++++++++++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 66 insertions(+), 27 deletions(-)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index 96abd34..e8b94fb 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -10,6 +10,8 @@
> >   * See the COPYING file in the top-level directory.
> >   */
> >
> > +#include <glib/gprintf.h>
> > +
> >  #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,64 @@ object_property_add(Object *obj, const char *name, const char *type,
> >      return prop;
> >  }
> >
> > +static void property_get_uint32_opaque(Object *obj, Visitor *v,
> > +                                   void *opaque, const char *name,
> > +                                   Error **errp)
> > +{
> > +    uint32_t value = (uintptr_t)opaque;
> > +    visit_type_uint32(v, &value, name, 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);
> > +    char *name_no_array;
> > +    ObjectProperty *ret, *count;
> > +    uintptr_t i;
> > +
> > +    if (name_len < 3 || memcmp(&name[name_len - 3], "[*]", 4)) {
> > +        return object_property_add_single(obj, name, type,
> > +                                          get, set, release, opaque, errp);
> > +    }
> > +
> > +    /* 20 characters for maximum possible uintptr_t (64-bit) */
> > +    name_no_array = g_malloc(name_len + 20);
> > +
> > +    count = object_property_find(obj, name, NULL);
> > +    if (count == NULL) {
> > +        /* This is very similar to object_property_add_uint32_ptr(), but:
> > +         * - Returns pointer
> > +         * - Will not recurse here so that we can use raw name with [*]
> > +         * - Allows us to use 'opaque' pointer itself as a storage, because
> > +         *   we want to store only a single integer which should not be
> > +         *   modified from outside.
> > +         */
> > +        count = object_property_add_single(obj, name, "uint32",
> > +                                           property_get_uint32_opaque, NULL,
> > +                                           NULL, NULL, &error_abort);
> > +    }
> > +
> > +    name_len -= 3;
> > +    memcpy(name_no_array, name, name_len);
> > +    i = (uintptr_t)count->opaque;
> > +
> > +    do {
> > +        g_sprintf(&name_no_array[name_len], "[%zu]", i++);
> > +
> > +        ret = object_property_add_single(obj, name_no_array, type, get, set,
> > +                                         release, opaque, NULL);
> > +    } while (!ret);
> > +
> > +    count->opaque = (void *)i;
> > +    g_free(name_no_array);
> > +    return ret;
> > +}
> > +
> >  ObjectProperty *object_property_find(Object *obj, const char *name,
> >                                       Error **errp)
> >  {
> > --
> > 1.9.5.msysgit.0
> >
> >
> >

> Ping!

 May be you could PULL it? Andreas seems to be down...

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Am 03.07.2015 um 15:20 schrieb Pavel Fedin:
>> Ping!
> 
>  May be you could PULL it? Andreas seems to be down...

Nack, I don't like the patch as is. Starting with the unoptimized code
movement combined with functional changes, up to the use of [*] for the
new property iiuc.

Andreas

Hello! Thank you very much for your feedback.

> Nack, I don't like the patch as is. Starting with the unoptimized code
> movement combined with functional changes

 Will it be OK if i split it into two parts:
 1. Restructuring, introduction of object_property_add_single()
 2. Introduction of cache property by itself.

> up to the use of [*] for the
> new property iiuc.

 What would you suggest instead? Personally i don't have much preferences, i used it just because it was already there.
 What about 'name#' ?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia