[Spice-devel] [PATCH] Remove X == TRUE tests for non-boolean values

[Christophe de Dinechin]

> On 27 Feb 2017, at 13:12, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> On Mon, Feb 27, 2017 at 06:58:47AM -0500, Frediano Ziglio wrote:
>>> 
>>> Using X == TRUE is fragile, since the input value is a uint8_t. It would be
>>> surprising if the value was set to 2 or 0xFF and the test failed.
>>> 
>>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com <mailto:dinechin at redhat.com>>
>> 
>> Acked-by: Frediano Ziglio <fziglio at redhat.com <mailto:fziglio at redhat.com>>
> 
> This apparently is going to cause warnings with gcc 7, see
> https://www.redhat.com/archives/libvir-list/2017-February/msg01199.html <https://www.redhat.com/archives/libvir-list/2017-February/msg01199.html>
> 
> if (dcc->priv->surface_client_created[surface_id] != 0) {} should work,
> or
> diff --git a/server/dcc-private.h b/server/dcc-private.h
> index 64b32a7..1cf3b0d 100644
> --- a/server/dcc-private.h
> +++ b/server/dcc-private.h
> @@ -51,7 +51,7 @@ struct DisplayChannelClientPrivate
>         int num_pixmap_cache_items;
>     } send_data;
> 
> -    uint8_t surface_client_created[NUM_SURFACES];
> +    bool surface_client_created[NUM_SURFACES];
>     QRegion surface_client_lossy_region[NUM_SURFACES];
> 
>     StreamAgent stream_agents[NUM_STREAMS];
> 
> which seems to be a more accurate type for 'surface_client_created’.

Looking at the declaration, I just noticed we pre-allocate 10000 surfaces. Why is this not dynamic allocation? Is it sane to allocate 250K statically with each DisplayChannelClientPrivate?

I’m tempted to replace this with a dynamic array of QRegion *, where a non-NULL pointer indicates that the client has been created. OK with that? I expect some operations will be faster because they won’t iterate over 10000 mostly empty surfaces. There will be one extra indirection when accessing the surface_client_lossy region, but x86_64 became pretty darn good at chasing pointers like that thanks to C++ vtables ;-)


Christophe
> 
> Chistophe
> 
> 
> 
>> 
>>> ---
>>> server/dcc.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/server/dcc.c b/server/dcc.c
>>> index afe37b1..7cfa72b 100644
>>> --- a/server/dcc.c
>>> +++ b/server/dcc.c
>>> @@ -398,7 +398,7 @@ static void
>>> add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
>>> 
>>>         surface_id = drawable->surface_deps[x];
>>>         if (surface_id != -1) {
>>> -            if (dcc->priv->surface_client_created[surface_id] == TRUE) {
>>> +            if (dcc->priv->surface_client_created[surface_id]) {
>>>                 continue;
>>>             }
>>>             dcc_create_surface(dcc, surface_id);
>>> @@ -407,7 +407,7 @@ static void
>>> add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
>>>         }
>>>     }
>>> 
>>> -    if (dcc->priv->surface_client_created[drawable->surface_id] == TRUE) {
>>> +    if (dcc->priv->surface_client_created[drawable->surface_id]) {
>>>         return;
>>>     }
>>> 
>> 
>> Frediano
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170227/a78a3378/attachment-0001.html>