Re: [Qemu-devel] [PATCH for-1.5] qom: optimize casting to leaf class and parent class

[Aurelien Jarno]

On Fri, May 10, 2013 at 05:58:23PM -0500, Anthony Liguori wrote:
> Aurelien Jarno <address@hidden> writes:
> 
> > On Fri, May 10, 2013 at 01:47:55PM -0500, Anthony Liguori wrote:
> >> Most QOM types use type_register_static but we still strdup the
> >> passed data.  However, the original pointers are useful because
> >> GCC is pretty good about collapsing strings so its very likely any
> >> use of the pointer will end up being that same address.
> >> 
> >> IOW, with a little trickery, we can compare types by just comparing
> >> strings and in fact that's what we do here.
> >> 
> >> We do this for the two most common cases, casting to a leaf class
> >> or to the parent class.
> >> 
> >> With these two changes, I see a decrease from around 2 hash table
> >> lookups to only a thousand with no run time lookups at all.
> >> 
> >> Cc: Paolo Bonzini <address@hidden>
> >> Cc: Aurelien Jarno <address@hidden>
> >> Cc: Andreas Färber <address@hidden>
> >> Reported-by: Aurelien Jarno <address@hidden>
> >> Signed-off-by: Anthony Liguori <address@hidden>
> >> ---
> >> Aurelien, could you please try this patch with your PPC test case?
> >> ---
> >>  qom/object.c | 16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/qom/object.c b/qom/object.c
> >> index 75e6aac..5ecfd28 100644
> >> --- a/qom/object.c
> >> +++ b/qom/object.c
> >> @@ -132,7 +132,13 @@ TypeImpl *type_register(const TypeInfo *info)
> >>  
> >>  TypeImpl *type_register_static(const TypeInfo *info)
> >>  {
> >> -    return type_register(info);
> >> +    TypeImpl *impl;
> >> +
> >> +    impl = type_register(info);
> >> +    impl->name = info->name;
> >> +    impl->parent = info->parent;
> >> +
> >> +    return impl;
> >>  }
> >>  
> >>  static TypeImpl *type_get_by_name(const char *name)
> >> @@ -449,10 +455,16 @@ Object *object_dynamic_cast_assert(Object *obj, 
> >> const char *typename)
> >>  ObjectClass *object_class_dynamic_cast(ObjectClass *class,
> >>                                         const char *typename)
> >>  {
> >> -    TypeImpl *target_type = type_get_by_name(typename);
> >> +    TypeImpl *target_type;
> >>      TypeImpl *type = class->type;
> >>      ObjectClass *ret = NULL;
> >>  
> >> +    if (type->name == typename || type->parent == typename) {
> >> +        return class;
> >> +    }
> >> +
> >> +    target_type = type_get_by_name(typename);
> >> +
> >>      if (!target_type) {
> >>          /* target class type unknown, so fail the cast */
> >>          return NULL;
> >
> > Unfortunately it doesn't fix the problem. I only see a 0.5% improvement,
> > which might be in the noise. I still see g_hash_table_lookup and
> > g_str_hash quite high in perf top.
> 
> I was afraid of this.  I assume the cast comes somewhere other than
> where the type was registered.
> 
> This patch should address that.  Could you post an image too?  Then I
> don't have to keep bugging you with updated patches.
> 

It doesn't fix the issue, I don't really see any improvement, and
g_hash_table_lookup + g_str_hash + __strcmp_sse42 are still high in 
perf top.

I have uploaded an image to reproduce the issue:

  http://temp.aurel32.net/debian_squeeze_powerpc_qom_casts.qcow2

You can run it with:

  qemu-system-ppc -snapshot -hda debian_squeeze_powerpc_qom_casts.qcow2

You can also add -nographic if you don't want the graphical interface,
the boot process won't show up, but after half a minute, you should get
to the login prompt. You can then login as user/user and run bench.sh,
though if you are using perf top, the libglib-2.0 functions are visible
even during the boot process.

It's a different image than the one I am using to test your patches, as
I didn't have a small ppc64 image ready. That said the issue is also
visible there.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net