qobject: Replace property list with GHashTable

PATCH In review Pavel Fedin 2 participants 2 messages Oct 6, 2015 – Oct 8, 2015

Share & tools

Pavel Fedin

author

2015-10-06 13:49 UTC #0 lore raw

PATCH v2 1/1 qobject: Replace property list with GHashTable

ARM GICv3 systems with large number of CPUs create lots of IRQ pins. Since every pin is represented as a property, number of these properties becomes very large. Every property add first makes sure there's no duplicates. Traversing the list becomes very slow, therefore qemu initialization takes significant time (several seconds for e. g. 16 CPUs).

This patch replaces list with GHashTable, making lookup very fast. The only drawback is that object_child_foreach() and object_child_foreach_recursive() cannot modify their objects during traversal, since GHashTableIter does not have modify-safe version. However, the code seems not to modify objects via these functions.

Signed-off-by Pavel Fedin <[email protected]>

---
v1 => v2:
- Fixed stupid bug in object_unparent(), use correct object

---
 include/qom/object.h |  4 +--
 qmp.c                |  8 +++--
 qom/object.c         | 98 +++++++++++++++++++++++-----------------------------
 vl.c                 |  4 ++-
 4 files changed, 54 insertions(+), 60 deletions(-)

include/qom/object.h +2 -2

index be7280c..b100923 100644
@@ -345,7 +345,7 @@ typedef struct ObjectProperty
     ObjectPropertyRelease *release;
     void *opaque;
 
-    QTAILQ_ENTRY(ObjectProperty) node;
+    Object *obj;
 } ObjectProperty;
 
 /**

@@ -405,7 +405,7 @@ struct Object
     /*< private >*/
     ObjectClass *class;
     ObjectFree *free;
-    QTAILQ_HEAD(, ObjectProperty) properties;
+    GHashTable *properties;
     uint32_t ref;
     Object *parent;
 };

qmp.c +6 -2

index 057a7cb..683f427 100644
@@ -207,6 +207,7 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
     Object *obj;
     bool ambiguous = false;
     ObjectPropertyInfoList *props = NULL;
+    GHashTableIter iter;
     ObjectProperty *prop;
 
     obj = object_resolve_path(path, &ambiguous);

@@ -220,7 +221,8 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
         return NULL;
     }
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
+    g_hash_table_iter_init(&iter, obj->properties);
+    while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
         ObjectPropertyInfoList *entry = g_malloc0(sizeof(*entry));
 
         entry->value = g_malloc0(sizeof(ObjectPropertyInfo));

@@ -499,6 +501,7 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
 {
     ObjectClass *klass;
     Object *obj;
+    GHashTableIter iter;
     ObjectProperty *prop;
     DevicePropertyInfoList *prop_list = NULL;

@@ -517,7 +520,8 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
 
     obj = object_new(typename);
 
-    QTAILQ_FOREACH(prop, &obj->properties, node) {
+    g_hash_table_iter_init(&iter, obj->properties);
+    while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
         DevicePropertyInfo *info;
         DevicePropertyInfoList *entry;

qom/object.c +43 ~~-55~~

187 lines not on this page · raw message

vl.c +3 -1

index e211f6a..7065d5f 100644
@@ -1506,13 +1506,15 @@ MachineInfoList *qmp_query_machines(Error **errp)
 
 static int machine_help_func(QemuOpts *opts, MachineState *machine)
 {
+    GHashTableIter iter;
     ObjectProperty *prop;
 
     if (!qemu_opt_has_help_opt(opts)) {
         return 0;
     }
 
-    QTAILQ_FOREACH(prop, &OBJECT(machine)->properties, node) {
+    g_hash_table_iter_init(&iter, OBJECT(machine)->properties);
+    while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
         if (!prop->set) {
             continue;
         }

Signature · 1 line

1.9.5.msysgit.0

Daniel P. Berrange

2015-10-08 10:20 UTC #1 lore raw

Nit-pick on subject line - s/qobject/qom/ and also copy Andreas Färber on the patch, since he's the primary maintainer of the QOM subsystem.

Quoted 28 lines · Pavel Fedin

ARM GICv3 systems with large number of CPUs create lots of IRQ pins. Since every pin is represented as a property, number of these properties becomes very large. Every property add first makes sure there's no duplicates. Traversing the list becomes very slow, therefore qemu initialization takes significant time (several seconds for e. g. 16 CPUs).
This patch replaces list with GHashTable, making lookup very fast. The only drawback is that object_child_foreach() and object_child_foreach_recursive() cannot modify their objects during traversal, since GHashTableIter does not have modify-safe version. However, the code seems not to modify objects via these functions.
Signed-off-by Pavel Fedin <[email protected]>
---
v1 => v2:
- Fixed stupid bug in object_unparent(), use correct object
---
 include/qom/object.h |  4 +--
 qmp.c                |  8 +++--
 qom/object.c         | 98 +++++++++++++++++++++++-----------------------------
 vl.c                 |  4 ++-
 4 files changed, 54 insertions(+), 60 deletions(-)
include/qom/object.h +0 -0
index be7280c..b100923 100644

include/qom/object.h typedef struct ObjectProperty · line 348

Quoted 5 lines

include/qom/object.h +1 -1

@@ -345,7 +345,7 @@ typedef struct ObjectProperty
     ObjectPropertyRelease *release;
     void *opaque;
 
-    QTAILQ_ENTRY(ObjectProperty) node;
+    Object *obj;

I have a patch which adds property registration against the class, so requiring ObjectProperty to have a back-poointer to an object instance is not desirable.

Quoted 5 lines

qom/object.c +0 -0
index 4805328..1c7c42a 100644

qom/object.c static void object_post_init_with_type(Object *obj, TypeImpl *ti) · line 335

Quoted 10 lines

qom/object.c +7 -0

@@ -326,6 +326,20 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
     }
 }
 
+static void property_destroy(gpointer data)
+{
+    ObjectProperty *prop = data;
+
+    if (prop->release) {
+        prop->release(prop->obj, prop->name, prop->opaque);
+    }

Instead of calling the release() callback here, use a hash iterator to call it from the object_finalize() method, before unref'ing the hash table, and also call it from objecT_property_del directly. That way you don't need to store the Object * back pointer in the ObjectProperty struct.

Quoted 27 lines

qom/object.c +7 -1

+
+    g_free(prop->name);
+    g_free(prop->type);
+    g_free(prop->description);
+    g_free(prop);
+}

@@ -410,7 +404,7 @@ static void object_finalize(void *data)
     Object *obj = data;
     TypeImpl *ti = obj->class->type;
 
-    object_property_del_all(obj);
+    g_hash_table_unref(obj->properties);
     object_deinit(obj, ti);
 
     g_assert(obj->ref == 0);

void object_property_del(Object *obj, const char *name, Error **errp) {

qom/object.c +1 -4

-    ObjectProperty *prop = object_property_find(obj, name, errp);
-    if (prop == NULL) {
+    if (g_hash_table_remove(obj->properties, name)) {
         return;
     }
 
-    if (prop->release) {
-        prop->release(obj, name, prop->opaque);

qom/object.c ObjectProperty *object_property_find(Object *obj, const char *name, · line 921

Quoted 10 lines

qom/object.c +1 -8

-    }
-
-    QTAILQ_REMOVE(&obj->properties, prop, node);
-
-    g_free(prop->name);
-    g_free(prop->type);
-    g_free(prop->description);
-    g_free(prop);
+    error_setg(errp, "Property '.%s' not found", name);
 }

Overall I think this is a good improvement to make

Regards,
Daniel

Signature · 4 lines

|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|