> On 27 Feb 2017, at 17:10, Frediano Ziglio <fzig...@redhat.com> wrote:
>
> On 27 Feb 2017, at 13:12, Christophe Fergeau <cferg...@redhat.com
> <mailto:cferg...@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 <dinec...@redhat.com
> <mailto:dinec...@redhat.com>>
>
> Acked-by: Frediano Ziglio <fzig...@redhat.com <mailto:fzig...@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 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.
Yes. Agreed.
>
> 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 :-)
Sorry. Did not mean to be. What I meant is that chasing pointers like a->b->c
is required to perform well for virtual function calls in C++ (this->foo()
translates into this->vptr->vtbl[n]()). At some point, it was a huge
performance hit for many workloads, because CPUs were not that good at
prefetching this kind of double-dereferences. But C++ and other similar
programming techniques with double indirection became so prevalent that CPUs
are now quite good at it.
> Could be true.
> I don't think the extra indirection is a big deal, it's mostly used on
> surface/channel creation/destroying.
OK
Christophe
>
>
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
