From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39340) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0QBP-000890-76 for qemu-devel@nongnu.org; Thu, 04 Jun 2015 04:07:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z0QBL-0001np-W3 for qemu-devel@nongnu.org; Thu, 04 Jun 2015 04:07:23 -0400 Received: from mail-qc0-f180.google.com ([209.85.216.180]:35495) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0QBL-0001ni-Qc for qemu-devel@nongnu.org; Thu, 04 Jun 2015 04:07:19 -0400 Received: by qczw4 with SMTP id w4so14847781qcz.2 for ; Thu, 04 Jun 2015 01:07:19 -0700 (PDT) MIME-Version: 1.0 Sender: peter.crosthwaite@petalogix.com In-Reply-To: <012f01d09e99$016ab660$04402320$@samsung.com> References: <012f01d09e99$016ab660$04402320$@samsung.com> Date: Thu, 4 Jun 2015 01:07:19 -0700 Message-ID: From: Peter Crosthwaite Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] Ping: [PATCH] qobject: object_property_add() performance improvement List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Fedin Cc: "qemu-devel@nongnu.org Developers" , Luiz Capitulino 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.com@nongnu.org [mailto:qemu-devel- >> bounces+p.fedin=samsung.com@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 look right. >> + 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? >> + /* 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 */ newline before */ >> + 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? Alternatively can you register the setter and use set/get instead of going hands on with the property opaque pointer? >> + NULL, v, &error_abort); >> + } >> + >> + name_len -= 2; >> + memcpy(name_no_array, name, name_len); Can you drop the "[" as well? You will have one less byte to memcpy and pick it up again in the sprintf so I dont see a huge perf advantage. Makes the code below more readable. >> + >> + for (i = *((uint32_t *)count->opaque); ; ++i) { Parentheses not needed. >> + g_sprintf(&name_no_array[name_len], "%d]", i); >> + Indentation. Regards, Peter >> + ret = object_property_add_single(obj, name_no_array, type, get, set, >> + release, opaque, NULL); >> + if (ret) { >> + break; >> + } >> + } >> + *((uint32_t *)count->opaque) = i + 1; >> + >> + g_free(name_no_array); >> + return ret; >> + } >> + >> + return object_property_add_single(obj, name, type, >> + get, set, release, opaque, errp); >> +} >> + >> ObjectProperty *object_property_find(Object *obj, const char *name, >> Error **errp) >> { >> -- >> 1.9.5.msysgit.0 >> > > >