Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

Am 10.05.2013 17:32, schrieb Alexander Graf:
> 
> On 10.05.2013, at 17:23, Andreas Färber wrote:
> 
>> Am 10.05.2013 17:06, schrieb Anthony Liguori:
>>> Andreas Färber <address@hidden> writes:
>>>
>>>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>>>> just like PowerPCCPU::env access in the opposite direction.
>>>>
>>>> This should slightly improve interrupt performance.
>>>>
>>>> Reported-by: Anthony Liguori <address@hidden>
>>>> Signed-off-by: Andreas Färber <address@hidden>
>>>
>>> Another option would be to leave it and do something like:
>>>
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 75e6aac..cba1d88 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -126,8 +126,13 @@ static TypeImpl *type_register_internal(const TypeInfo 
>>> *info)
>>>
>>> TypeImpl *type_register(const TypeInfo *info)
>>> {
>>> +    TypeImpl *impl;
>>> +
>>>     assert(info->parent);
>>> -    return type_register_internal(info);
>>> +    impl = type_register_internal(info);
>>> +    g_free(impl->name);
>>> +    impl->name = info->name;
>>> +    return impl;
>>> }
>>>
>>> TypeImpl *type_register_static(const TypeInfo *info)
>>> @@ -449,10 +490,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) {
>>> +        return class;
>>> +    }
>>> +
>>> +    target_type = type_get_by_name(typename);
>>> +
>>>     if (!target_type) {
>>>         /* target class type unknown, so fail the cast */
>>>         return NULL;
>>>
>>> Which makes casting an object to it's concrete class free.
>>
>> Doesn't help here since concrete class is POWER7_v2.1-ppc64-cpu whereas
>> we're casting to ppc64-cpu, with two-level hierarchy by now:
>> POWER7_v2.1 -> POWER7 -> ppc64 -> device -> object.
> 
> How much performance penalty do we get from this?

Not sure which "this" you are referring to, but in general dynamic_cast
does a check for interfaces (which we don't have) and then iterates
through the hierarchy with string comparisons, i.e. negative, negative,
positive for POWERPC_CPU(). My original patch here dropped this penalty
for ppc_env_get_cpu(); CPU() would still result in negative, negative,
negative, positive.

Personally I wouldn't oppose dropping these checks for release builds as
proposed by Paolo in his series; for me, the value of POWERPC_CPU() is
being closer to an OO cast than any container_of()-style expressions.

But I can also see Anthony's point that we should try to optimize
dynamic_cast rather than circumventing it.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg