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 <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

---
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(-)

qom/object.c +66 -28

index 96abd34..e8b94fb 100644
@@ -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

-----Original Message-----
From: [email protected] [mailto:qemu-devel- [email protected]] 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

Quoted 153 lines · Pavel Fedin

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 <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

---
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(-)

qom/object.c +66 -28

index 96abd34..e8b94fb 100644
@@ -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!

Quoted 1 line

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.