Re: [Qemu-devel] [PATCH v3] QOM: object_property_add() performance improvement

[Pavel Fedin]

 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: address@hidden [mailto:qemu-devel-
> address@hidden 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 <address@hidden> 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 <address@hidden>
> > Reviewed-by: Peter Crosthwaite <address@hidden>
> > ---
> >  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
> >
> >
> >