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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 11/05/2013 00:58, Anthony Liguori ha scritto:
> Aurelien Jarno <aurelien@aurel32.net> 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 <pbonzini@redhat.com>
>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>> Cc: Andreas Färber <afaerber@suse.de>
>>> Reported-by: Aurelien Jarno <aurelien@aurel32.net>
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>> ---
>>> 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;
>>>  }

This is ok with a comment.

>>>  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;
>>> +    }

I prefer my patch 3/9.  With the hunk above, it works fine for the
simple case of casts in a device model's callbacks (testing type->parent
would almost always fail, so it is not worthwhile).

Unfortunately, strcmp is just as bad as a hashtable lookup (both are
O(n) in the size the string, instead of O(1)).

Paolo

>>> +    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.
> 
> 
> 
> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> -- 
>> Aurelien Jarno	                        GPG: 1024D/F1BCDB73
>> aurelien@aurel32.net                 http://www.aurel32.net