[27/32] PPC: Introduce an alias cache for faster lookups

When running QEMU with "-cpu ?" we walk through every alias for every
target CPU we know about. This takes several seconds on my very fast
host system.

Let's introduce a class object cache in the alias table. Using that we
don't have to go through the tedious work of finding our target class.
Instead, we can just go directly from the alias name to the target class
pointer.

This patch brings -cpu "?" to reasonable times again.

Before:
  real    0m4.716s

After:
  real    0m0.025s

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-ppc/cpu-models.c     |  2 +-
 target-ppc/cpu-models.h     |  3 ++-
 target-ppc/translate_init.c | 32 +++++++++++++++++++++++++++-----
 3 files changed, 30 insertions(+), 7 deletions(-)

Am 30.06.2013 03:45, schrieb Alexander Graf:
> When running QEMU with "-cpu ?" we walk through every alias for every
> target CPU we know about. This takes several seconds on my very fast
> host system.
> 
> Let's introduce a class object cache in the alias table. Using that we
> don't have to go through the tedious work of finding our target class.
> Instead, we can just go directly from the alias name to the target class
> pointer.
> 
> This patch brings -cpu "?" to reasonable times again.
> 
> Before:
>   real    0m4.716s
> 
> After:
>   real    0m0.025s
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

I had objected to this patch being not the right solution to the problem.

> ---
>  target-ppc/cpu-models.c     |  2 +-
>  target-ppc/cpu-models.h     |  3 ++-
>  target-ppc/translate_init.c | 32 +++++++++++++++++++++++++++-----
>  3 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
> index 17f56b7..9bb68c8 100644
> --- a/target-ppc/cpu-models.c
> +++ b/target-ppc/cpu-models.c
> @@ -1227,7 +1227,7 @@
>  /***************************************************************************/
>  /* PowerPC CPU aliases                                                     */
>  
> -const PowerPCCPUAlias ppc_cpu_aliases[] = {
> +PowerPCCPUAlias ppc_cpu_aliases[] = {
>      { "403", "403GC" },
>      { "405", "405D4" },
>      { "405CR", "405CRc" },
> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
> index a94f835..262ca47 100644
> --- a/target-ppc/cpu-models.h
> +++ b/target-ppc/cpu-models.h
> @@ -31,9 +31,10 @@
>  typedef struct PowerPCCPUAlias {
>      const char *alias;
>      const char *model;
> +    ObjectClass *klass;

And please don't spread this deliberate misspelling. When I did, I was
flamed and the solution was to use oc for ObjectClass, cc for CPUClass,
etc. Your patch still keeps traversing the class list with O(n).

>  } PowerPCCPUAlias;
>  
> -extern const PowerPCCPUAlias ppc_cpu_aliases[];
> +extern PowerPCCPUAlias ppc_cpu_aliases[];
>  
>  /*****************************************************************************/
>  /* PVR definitions for most known PowerPC                                    */
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index f01e9e7..45b4053 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -7934,6 +7934,28 @@ static gint ppc_cpu_compare_class_name(gconstpointer a, gconstpointer b)
>  
>  #include <ctype.h>
>  
> +static ObjectClass *ppc_cpu_class_by_name(const char *name);
> +
> +static ObjectClass *ppc_cpu_class_by_alias(PowerPCCPUAlias *alias)
> +{
> +    ObjectClass *invalid_class = (void*)ppc_cpu_class_by_alias;
> +
> +    /* Cache target class lookups in the alias table */
> +    if (!alias->klass) {
> +        alias->klass = ppc_cpu_class_by_name(alias->model);
> +        if (!alias->klass) {
> +            /* Fast check for non-existing aliases */
> +            alias->klass = invalid_class;
> +        }
> +    }
> +
> +    if (alias->klass == invalid_class) {
> +        return NULL;
> +    } else {
> +        return alias->klass;
> +    }
> +}

Instead of saving bogus values with meaning "no class" we should drop
the ifdef'fery and make all types available. Our plan was to add one or
more flags to PowerPCCPUClass.

Andreas

> +
>  static ObjectClass *ppc_cpu_class_by_name(const char *name)
>  {
>      GSList *list, *item;
> @@ -7961,7 +7983,7 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name)
>  
>      for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
>          if (strcmp(ppc_cpu_aliases[i].alias, name) == 0) {
> -            return ppc_cpu_class_by_name(ppc_cpu_aliases[i].model);
> +            return ppc_cpu_class_by_alias(&ppc_cpu_aliases[i]);
>          }
>      }
>  
> @@ -8051,8 +8073,8 @@ static void ppc_cpu_list_entry(gpointer data, gpointer user_data)
>      (*s->cpu_fprintf)(s->file, "PowerPC %-16s PVR %08x\n",
>                        name, pcc->pvr);
>      for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
> -        const PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
> -        ObjectClass *alias_oc = ppc_cpu_class_by_name(alias->model);
> +        PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
> +        ObjectClass *alias_oc = ppc_cpu_class_by_alias(alias);
>  
>          if (alias_oc != oc) {
>              continue;
> @@ -8119,12 +8141,12 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>      g_slist_free(list);
>  
>      for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
> -        const PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
> +        PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
>          ObjectClass *oc;
>          CpuDefinitionInfoList *entry;
>          CpuDefinitionInfo *info;
>  
> -        oc = ppc_cpu_class_by_name(alias->model);
> +        oc = ppc_cpu_class_by_alias(alias);
>          if (oc == NULL) {
>              continue;
>          }
>

On 30.06.2013, at 08:25, Andreas Färber wrote:

> Am 30.06.2013 03:45, schrieb Alexander Graf:
>> When running QEMU with "-cpu ?" we walk through every alias for every
>> target CPU we know about. This takes several seconds on my very fast
>> host system.
>> 
>> Let's introduce a class object cache in the alias table. Using that we
>> don't have to go through the tedious work of finding our target class.
>> Instead, we can just go directly from the alias name to the target class
>> pointer.
>> 
>> This patch brings -cpu "?" to reasonable times again.
>> 
>> Before:
>>  real    0m4.716s
>> 
>> After:
>>  real    0m0.025s
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> I had objected to this patch being not the right solution to the problem.

Not on the mailing list FWIW. It's a real solution to a real problem. Just revert it when the final all-surpassing solution comes along. Until then, at least -cpu ? works fast.

> 
>> ---
>> target-ppc/cpu-models.c     |  2 +-
>> target-ppc/cpu-models.h     |  3 ++-
>> target-ppc/translate_init.c | 32 +++++++++++++++++++++++++++-----
>> 3 files changed, 30 insertions(+), 7 deletions(-)
>> 
>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>> index 17f56b7..9bb68c8 100644
>> --- a/target-ppc/cpu-models.c
>> +++ b/target-ppc/cpu-models.c
>> @@ -1227,7 +1227,7 @@
>> /***************************************************************************/
>> /* PowerPC CPU aliases                                                     */
>> 
>> -const PowerPCCPUAlias ppc_cpu_aliases[] = {
>> +PowerPCCPUAlias ppc_cpu_aliases[] = {
>>     { "403", "403GC" },
>>     { "405", "405D4" },
>>     { "405CR", "405CRc" },
>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>> index a94f835..262ca47 100644
>> --- a/target-ppc/cpu-models.h
>> +++ b/target-ppc/cpu-models.h
>> @@ -31,9 +31,10 @@
>> typedef struct PowerPCCPUAlias {
>>     const char *alias;
>>     const char *model;
>> +    ObjectClass *klass;
> 
> And please don't spread this deliberate misspelling. When I did, I was
> flamed and the solution was to use oc for ObjectClass, cc for CPUClass,
> etc.

ok, s/klass/oc/g then.

> Your patch still keeps traversing the class list with O(n).

So?

> 
>> } PowerPCCPUAlias;
>> 
>> -extern const PowerPCCPUAlias ppc_cpu_aliases[];
>> +extern PowerPCCPUAlias ppc_cpu_aliases[];
>> 
>> /*****************************************************************************/
>> /* PVR definitions for most known PowerPC                                    */
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index f01e9e7..45b4053 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -7934,6 +7934,28 @@ static gint ppc_cpu_compare_class_name(gconstpointer a, gconstpointer b)
>> 
>> #include <ctype.h>
>> 
>> +static ObjectClass *ppc_cpu_class_by_name(const char *name);
>> +
>> +static ObjectClass *ppc_cpu_class_by_alias(PowerPCCPUAlias *alias)
>> +{
>> +    ObjectClass *invalid_class = (void*)ppc_cpu_class_by_alias;
>> +
>> +    /* Cache target class lookups in the alias table */
>> +    if (!alias->klass) {
>> +        alias->klass = ppc_cpu_class_by_name(alias->model);
>> +        if (!alias->klass) {
>> +            /* Fast check for non-existing aliases */
>> +            alias->klass = invalid_class;
>> +        }
>> +    }
>> +
>> +    if (alias->klass == invalid_class) {
>> +        return NULL;
>> +    } else {
>> +        return alias->klass;
>> +    }
>> +}
> 
> Instead of saving bogus values with meaning "no class" we should drop
> the ifdef'fery and make all types available. Our plan was to add one or
> more flags to PowerPCCPUClass.

I'm not sure I really want those types there already. There's some serious further refactoring in the CPU initialization code needed to group common registration between different CPUs together. Things are way too much copy&paste today still.


Alex