On 27 Feb 2017, at 13:12, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxx>
Acked-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
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
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel