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

[Frediano Ziglio]

> > 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 >
> > > 
> > 
> 

> > > Acked-by: Frediano Ziglio < 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
> 

> > 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 think mostly old style pool allocations. Considering that usually we use only 1 surface would be indeed much smaller. 

> 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

I would use a new structure like 

typedef struct { 
QRegion region; 
} RedClientSurfaceInfo; 

make source code a bit longer (not the binary) but can be easily extended and it's easier to 
understand a dcc->client_surface_info[i] != NULL than a dcc->surface_client_lossy_region[i] != NULL. 

> 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 ;-)

Last sentence is a bit cryptic I think :-) 
Could be true. 
I don't think the extra indirection is a big deal, it's mostly used on surface/channel creation/destroying. 

> 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 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170227/d5062f85/attachment.html>