[v4,2/2] QOM: object_property_add() performance improvement

On 27/07/2015 16:57, Andreas Färber wrote:
>> >  I am absolutely fine with absolutely anything. Suggest what you like and i'll change it.
> Paolo suggested ...-count on #qemu, but I would prefer ...-max or so, as
> the number could differ when some property gets deleted.

Yes, I agree -max is better.

I'm just asking myself whether this is really necessary.  Is the
automagic [*] really needed in this case?  Can it just do:


?

Paolo

> On the other hand, since this is not a user-added property, using a
> reserved character such as '#' would avoid clashes with user-added
> properties, as long as tools handle accessing that property okay.

On 27/07/2015 17:06, Paolo Bonzini wrote:
> 
> 
> On 27/07/2015 16:57, Andreas Färber wrote:
>>>>  I am absolutely fine with absolutely anything. Suggest what you like and i'll change it.
>> Paolo suggested ...-count on #qemu, but I would prefer ...-max or so, as
>> the number could differ when some property gets deleted.
> 
> Yes, I agree -max is better.
> 
> I'm just asking myself whether this is really necessary.  Is the
> automagic [*] really needed in this case?  Can it just do:
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index b2f404a..19bfee1 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -415,19 +415,19 @@ static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
>  void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
>                               const char *name, int n)
>  {
> -    int i;
> +    int i, j;
>      NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
> -    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-in");
>  
>      assert(gpio_list->num_out == 0 || !name);
>      gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
>                                       dev, n);
>  
>      for (i = gpio_list->num_in; i < gpio_list->num_in + n; i++) {
> +        char *propname = g_strdup_printf("%s[%d]", name ? name : "unnamed-gpio-in", j++);
>          object_property_add_child(OBJECT(dev), propname,
>                                    OBJECT(gpio_list->in[i]), &error_abort);
> +        g_free(propname);
>      }
> -    g_free(propname);
>  
>      gpio_list->num_in += n;
>  }
> 
> ?

... and the same in qdev_init_gpio_out_named.

Paolo

Hello!

> I'm just asking myself whether this is really necessary.  Is the
> automagic [*] really needed in this case?  

 In this particular case, perhaps, not. However, this automagic is used not only with GPIOs. It is used also with memory regions, as well as with some other places. Also, i thought that there could be some hard to notice problems, when, for example, we first add unnamed-gpio-in[0...1023], then add another 1024 pins, where count again goes from 0 to 1023. And we would get collision and failure, unless we know, that we already have 1024 objects with this name. So, just for better safety, i decided to fix the mechanism itself instead of changing use cases.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

On 27/07/2015 17:19, Pavel Fedin wrote:
>>> I'm just asking myself whether this is really necessary.  Is the 
>>> automagic [*] really needed in this case?
> In this particular case, perhaps, not. However, this automagic is
> used not only with GPIOs. It is used also with memory regions, as
> well as with some other places. Also, i thought that there could be
> some hard to notice problems, when, for example, we first add
> unnamed-gpio-in[0...1023], then add another 1024 pins, where count
> again goes from 0 to 1023. And we would get collision and failure,
> unless we know, that we already have 1024 objects with this name.

But IIUC qdev_init_gpio_in/out (the non-named variants) should only be
called once.  So if it breaks it's a feature.

Paolo

> So,
> just for better safety, i decided to fix the mechanism itself instead
> of changing use cases.

> > Also, i thought that there could be
> > some hard to notice problems, when, for example, we first add
> > unnamed-gpio-in[0...1023], then add another 1024 pins, where count
> > again goes from 0 to 1023. And we would get collision and failure,
> > unless we know, that we already have 1024 objects with this name.
> 
> But IIUC qdev_init_gpio_in/out (the non-named variants) should only be
> called once.  So if it breaks it's a feature.

 Ok ok ok...
 I can try to reengineer this and see what happens. If it works fine, will such rework be accepted? [*] expansion would still be slow, but we could deprecate it.

 I have just done a search of "[*]" across all *.c files, and here is what i came up with:
1. memory_region_init()
2. xlnx_zynqmp_init()
3. qdev_init_gpio_in_named()
4. qdev_init_gpio_out_named()
5. qdev_connect_gpio_out_named()
6. spapr_dr_connector_new()

 Cases 2, 3, 4 can be reengineered for sure. The rest - i don't know, however perhaps they are not common cases. I think (1) could also be problematic. How many regions with the same name can we have?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

On 28/07/2015 08:45, Pavel Fedin wrote:
>  I can try to reengineer this and see what happens. If it works fine, will such rework be accepted? [*] expansion would still be slow, but we could deprecate it.
> 
>  I have just done a search of "[*]" across all *.c files, and here is what i came up with:
> 1. memory_region_init()
> 2. xlnx_zynqmp_init()
> 3. qdev_init_gpio_in_named()
> 4. qdev_init_gpio_out_named()
> 5. qdev_connect_gpio_out_named()
> 6. spapr_dr_connector_new()
> 
>  Cases 2, 3, 4 can be reengineered for sure. The rest - i don't know, however perhaps they are not common cases. I think (1) could also be problematic. How many regions with the same name can we have?

Just worry about 3 and 4, they are the big offenders.

Paolo