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.fe...@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 :|
