Re: [Qemu-devel] [PATCH v2] qom: object_property_add() performance improvement

[Peter Crosthwaite]

On Wed, Jun 10, 2015 at 12:51 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.

blank line between paras.

> 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).\

10 secs is still pretty brutal for machine init. Got any clues for
where the rest of the time is going?

>
> Signed-off-by: Pavel Fedin <address@hidden>
> ---
> Changes since v1:
> - Fixed typos in comments
> - Fixed code style
> - Got rid of g_malloc0() for property storage space, now use 'opaque' field 
> itself as a
> data. Getter is still present for possible inspection from outside (e. g. 
> monitor).
> ---
>  qom/object.c | 95 
> +++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 68 insertions(+), 27 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 96abd34..63773e4 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,66 @@ 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);
> +}
> +
> +

one less blank.

> +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;
> +    int i;

Just define it as a uintptr_t to avoid the casts.

> +
> +    if (name_len < 3 || memcmp(&name[name_len - 3], "[*]", 4)) {
> +        return object_property_add_single(obj, name, type,
> +                                          get, set, release, opaque, errp);
> +

remove this blank.

otherwise,

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter

> +    }
> +
> +    /* 10 characters for maximum possible integer number */
> +    name_no_array = g_malloc(name_len + 10);
> +
> +    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], "[%d]", i++);
> +
> +        ret = object_property_add_single(obj, name_no_array, type, get, set,
> +                                         release, opaque, NULL);
> +    } while (!ret);
> +
> +    count->opaque = (void *)(intptr_t)i;
> +    g_free(name_no_array);
> +    return ret;
> +}
> +
>  ObjectProperty *object_property_find(Object *obj, const char *name,
>                                       Error **errp)
>  {
> --
> 1.9.5.msysgit.0
>
>
>