Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Nov 05, 2015 at 07:05:48PM +0100, Andreas Färber wrote:
> Am 13.10.2015 um 14:37 schrieb Daniel P. Berrange:
> > From: Pavel Fedin <p.fedin@samsung.com>
> > 
> > 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.
> 
> "modify objects" seems a little misleading here; from what I see only
> adding or removing properties (including child<>s) is forbidden, right?
> Modifying one ObjectProperty or its value should still be okay.

Yeah, that's correct.

> >  2 files changed, 64 insertions(+), 44 deletions(-)
> [...]
> > diff --git a/qom/object.c b/qom/object.c
> > index 7dace59..dd01652 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -68,7 +68,7 @@ struct TypeImpl
> >  };
> >  
> >  struct ObjectPropertyIterator {
> > -    ObjectProperty *next;
> > +    GHashTableIter iter;
> >  };
> >  
> >  static Type type_interface;
> > @@ -330,6 +330,16 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
> >      }
> >  }
> >  
> > +static void property_free(gpointer data)
> 
> Bikeshed: We might call this object_property_free() unless there's a
> precedence for property_...?

Sure, object_property_free() sounds fine.

> 
> > +{
> > +    ObjectProperty *prop = data;
> > +
> > +    g_free(prop->name);
> > +    g_free(prop->type);
> > +    g_free(prop->description);
> > +    g_free(prop);
> > +}
> > +
> >  void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
> >  {
> >      Object *obj = data;
> [...]
> > @@ -363,29 +374,35 @@ static inline bool object_property_is_child(ObjectProperty *prop)
> >  
> >  static void object_property_del_all(Object *obj)
> >  {
> > -    while (!QTAILQ_EMPTY(&obj->properties)) {
> > -        ObjectProperty *prop = QTAILQ_FIRST(&obj->properties);
> > -
> > -        QTAILQ_REMOVE(&obj->properties, prop, node);
> > +    ObjectProperty *prop;
> > +    GHashTableIter iter;
> > +    gpointer key, value;
> >  
> > +    g_hash_table_iter_init(&iter, obj->properties);
> > +    while (g_hash_table_iter_next(&iter, &key, &value)) {
> > +        prop = value;
> >          if (prop->release) {
> >              prop->release(obj, prop->name, prop->opaque);
> >          }
> 
> Why is this not in property_free(), too? Is there a timing difference?

To have this be part of property_free() would require that the
ObjectProperty class have a back-pointer to the Object * that
owns it. We want to use ObjectProperty from ObjectClass *
too though, so we don't really want to have such a Object *
backpointer.

> > @@ -924,7 +940,7 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
> >  ObjectPropertyIterator *object_property_iter_init(Object *obj)
> >  {
> >      ObjectPropertyIterator *ret = g_new0(ObjectPropertyIterator, 1);
> > -    ret->next = QTAILQ_FIRST(&obj->properties);
> > +    g_hash_table_iter_init(&ret->iter, obj->properties);
> >      return ret;
> >  }
> >  
> 
> Is it intentional that our iterator pattern differs?

If you use the GHashTable approach, then the caller needs to allocate
the ObjectPropertyIterator which means it has to be a public typedef.
I wanted to keep the ObjectPropertyIterator struct contents private,
and have object_property_iter_init() return the allocated struct.

> 
> > @@ -940,31 +956,27 @@ void object_property_iter_free(ObjectPropertyIterator *iter)
> >  
> >  ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter)
> >  {
> > -    ObjectProperty *ret = iter->next;
> > -    if (ret) {
> > -        iter->next = QTAILQ_NEXT(iter->next, node);
> > +    gpointer key, val;
> > +    if (!g_hash_table_iter_next(&iter->iter, &key, &val)) {
> > +        return NULL;
> >      }
> > -    return ret;
> > +    return val;
> >  }
> >  
> >  
> >  void object_property_del(Object *obj, const char *name, Error **errp)
> >  {
> > -    ObjectProperty *prop = object_property_find(obj, name, errp);
> > -    if (prop == NULL) {
> > +    ObjectProperty *prop = g_hash_table_lookup(obj->properties, name);
> > +
> > +    if (!prop) {
> > +        error_setg(errp, "Property '.%s' not found", name);
> 
> Is this a behavioral change?

No,  object_property_find() will return exactly the same error as
this new code does.


> > @@ -1484,11 +1496,13 @@ void object_property_add_const_link(Object *obj, const char *name,
> >  gchar *object_get_canonical_path_component(Object *obj)
> >  {
> >      ObjectProperty *prop = NULL;
> > +    GHashTableIter iter;
> >  
> >      g_assert(obj);
> >      g_assert(obj->parent != NULL);
> >  
> > -    QTAILQ_FOREACH(prop, &obj->parent->properties, node) {
> > +    g_hash_table_iter_init(&iter, obj->parent->properties);
> > +    while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
> 
> Is this cast needed?

Probably not, as any pointer should coerce to void * without an
explicit cast, unless it had a 'const' involved.

Regards,
Daniel
-- 
|: 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 :|