Message ID | 017b01d0a809$0f682580$2e387080$@samsung.com |
---|---|
State | New |
Headers | show |
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
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) {