Re: [Spice-devel] [spice-gtk v2] channel-display: Use GHashTable to keep stream's structure
On Tue, 2018-03-13 at 08:53 +0100, Victor Toso wrote: > Hi, > > On Mon, Mar 12, 2018 at 05:04:44PM +0100, Lukáš Hrázký wrote: > > > I don't have numbers as I didn't consider it to be > > > significant at the time. My main concern was that it relies > > > on something not documented in the protocol itself (stream's > > > ID being a small number). > > > > But that is the old code relying on that, right? Any other > > reason than that to make this change? (I was thinking it was > > some preparation for adding the measurements) > > Not really. Just that 'id' field matches better with a hash table > then a oversized array. > > > I mean (theoretically! :)) if your patch was how the code was, > > somebody could write a patch to change it to what is the > > current (master) way with a message like: > > > > "Avoid the overhead of the hashtable by ensuring small stream > > IDs in the protocol and use an array to store the stream data." > > > > :D Not saying what you did is wrong, just that it depends on > > the context (which I know little of :)). > > Right > > > > > Might be a non-issue, just pointing it out as probably the only > > > > real consideration with this patch? > > > > > > Evaluating the difference between accessing an array an hash > > > table seems an overkill to me but, yeah, can be done... > > > > Not exactly what I meant. What I meant would be evaluating the > > difference between the whole codepath of processing one frame > > to the accessing of the hash table. We can safely say accessing > > an array is negligible :) > > I'm confused. You just mentioned above that: > > | "Avoid the overhead of the hashtable by ensuring small > | stream IDs in the protocol and use an array to store the > | stream data." > > Could be a possible patch, etc. This patch changes from array to > hash table and that would be the only significant change on > "whole codepath of processing one frame" wouldn't it? Not sure we understand each other, you get it was just a thought game, right? :) Say your patch was merged, someone could make a patch to change it back and make an argument similar to what I wrote... It's an argument in the opposite direction. In this case, the hashtable overhead seems negligible and therefore it should be a better solution. > > So if the accessing of the hash table was say ~ 1% or 2% of the > > frame processing time, I would say in this case it's not worth > > the slowdown (of course YMMV with the number). The reason is we > > can quite safely ensure the array indexed by IDs is safe by > > asserting the size is lower than something reasonable and then > > (unless there is another reason) the hashtable has very little > > advantage. > > > > Of course I have no clue what is going on in the processing of > > one frame, it may well be that the number is like 0.001% and > > then it's a non-issue :) > > So, this holds the display_stream structure which holds the > video_decoder field. By protocol, we can have multiple streams > and each stream could be in different formats like VP8, VP9, > H264, H265 and from a stream-id we need to get its display_stream > to add that payload to be decoded, etc. > > e.g: > * st->video_decoder->queue_frame(st->video_decoder, frame, ...); > * st->video_decoder->reschedule(st->video_decoder); > > > > I'm actually touching this part of the code thinking on > > > measurements but from the decoder's perspective (e.g from > > > SpiceFrame creation, decoding/rendering and deallocation). > > > > Not following here, I'm still not that familiar with the code. > > Needless to have a lenghty discussion with me while I keep > > guessing though :D I just wanted to point the one thing and I'm > > sure others can chime in with opinions on what's better and > > whether numbers are needed :) > > Sure. What I mean is that, I'm trying to measure from the moment > we create and queue a SpiceFrame to the moment it is freed. We > can have several things going on on the Decoding side and it > would be cool to measure it, let me give you an example: > > We have a mjpeg decoder but we could also decode with GStreamer. > GStreamer has more then one decoding element and even more > elements in its pipeline but, would be nice to do a fair > comparison and that's what I would like to achieve. When you talk about it, I have a feeling one hashing function really doesn't matter here :) > Another example is by having the almost exactly pipeline but > using Snir's patch [0] that renders on GPU. How much gain would > that be? > > [0] https://lists.freedesktop.org/archives/spice-devel/2018-March/042688.html > > This is OT for this patch of course, I'm just explaining because > I don't see a reason why not to. The reason I sent this patch is: > > 1) As stream-id field is not documented to be ascending from 0 > (and in fact, it isn't with host doing the streaming) the current > code has flaws that could be addressed. > > 2) I end up sending patches for things that ov
Re: [Spice-devel] [spice-gtk v2] channel-display: Use GHashTable to keep stream's structure
Hi, On Mon, Mar 12, 2018 at 05:04:44PM +0100, Lukáš Hrázký wrote: > > I don't have numbers as I didn't consider it to be > > significant at the time. My main concern was that it relies > > on something not documented in the protocol itself (stream's > > ID being a small number). > > But that is the old code relying on that, right? Any other > reason than that to make this change? (I was thinking it was > some preparation for adding the measurements) Not really. Just that 'id' field matches better with a hash table then a oversized array. > I mean (theoretically! :)) if your patch was how the code was, > somebody could write a patch to change it to what is the > current (master) way with a message like: > > "Avoid the overhead of the hashtable by ensuring small stream > IDs in the protocol and use an array to store the stream data." > > :D Not saying what you did is wrong, just that it depends on > the context (which I know little of :)). Right > > > Might be a non-issue, just pointing it out as probably the only > > > real consideration with this patch? > > > > Evaluating the difference between accessing an array an hash > > table seems an overkill to me but, yeah, can be done... > > Not exactly what I meant. What I meant would be evaluating the > difference between the whole codepath of processing one frame > to the accessing of the hash table. We can safely say accessing > an array is negligible :) I'm confused. You just mentioned above that: | "Avoid the overhead of the hashtable by ensuring small | stream IDs in the protocol and use an array to store the | stream data." Could be a possible patch, etc. This patch changes from array to hash table and that would be the only significant change on "whole codepath of processing one frame" wouldn't it? > So if the accessing of the hash table was say ~ 1% or 2% of the > frame processing time, I would say in this case it's not worth > the slowdown (of course YMMV with the number). The reason is we > can quite safely ensure the array indexed by IDs is safe by > asserting the size is lower than something reasonable and then > (unless there is another reason) the hashtable has very little > advantage. > > Of course I have no clue what is going on in the processing of > one frame, it may well be that the number is like 0.001% and > then it's a non-issue :) So, this holds the display_stream structure which holds the video_decoder field. By protocol, we can have multiple streams and each stream could be in different formats like VP8, VP9, H264, H265 and from a stream-id we need to get its display_stream to add that payload to be decoded, etc. e.g: * st->video_decoder->queue_frame(st->video_decoder, frame, ...); * st->video_decoder->reschedule(st->video_decoder); > > I'm actually touching this part of the code thinking on > > measurements but from the decoder's perspective (e.g from > > SpiceFrame creation, decoding/rendering and deallocation). > > Not following here, I'm still not that familiar with the code. > Needless to have a lenghty discussion with me while I keep > guessing though :D I just wanted to point the one thing and I'm > sure others can chime in with opinions on what's better and > whether numbers are needed :) Sure. What I mean is that, I'm trying to measure from the moment we create and queue a SpiceFrame to the moment it is freed. We can have several things going on on the Decoding side and it would be cool to measure it, let me give you an example: We have a mjpeg decoder but we could also decode with GStreamer. GStreamer has more then one decoding element and even more elements in its pipeline but, would be nice to do a fair comparison and that's what I would like to achieve. Another example is by having the almost exactly pipeline but using Snir's patch [0] that renders on GPU. How much gain would that be? [0] https://lists.freedesktop.org/archives/spice-devel/2018-March/042688.html This is OT for this patch of course, I'm just explaining because I don't see a reason why not to. The reason I sent this patch is: 1) As stream-id field is not documented to be ascending from 0 (and in fact, it isn't with host doing the streaming) the current code has flaws that could be addressed. 2) I end up sending patches for things that overall bothers me so I can either fix something in the code or understand, from other's views, where I'm mistaken. Again, thanks for the review and discussion! Cheers, toso signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v2] channel-display: Use GHashTable to keep stream's structure
On Mon, 2018-03-12 at 16:25 +0100, Victor Toso wrote:
> Hi,
>
> Thanks for taking a look!
>
> On Mon, Mar 12, 2018 at 04:13:30PM +0100, Lukáš Hrázký wrote:
> > On Wed, 2018-03-07 at 09:17 +0100, Victor Toso wrote:
> > > From: Victor Toso
> > >
> > > The major issue with the current approach is that it relies on
> > > the ID from SpiceMsgDisplayStreamCreate to create the smallest 2^n
> > > sized array that could fit the stream's id as index.
> > >
> > > This is potentially dangerous as the ID value is not documented and
> > > could have any uint32_t value. We depend on Spice server's
> > > implementation which currently defines max of 50 streams.
> > >
> > > > /** Maximum number of streams created by spice-server */
> > > > #define NUM_STREAMS 50
> > >
> > > The first ID has the value of 49* while the c->streams array would be
> > > allocated to 64. We could only benefit from allocating on high
> > > creating/removal of streams, which is not the case.
> > >
> > > We can improve the above situations by using a hash table.
> > >
> > > Signed-off-by: Victor Toso
> > > ---
> > > Changes from v2:
> > > * using g_direct_hash/equal instead of the integer
> > > conterpart.
> > > * changed the signature from int to uint32_t for id in destroy_stream()
> > >
> > > src/channel-display.c | 73
> > > ++-
> > > 1 file changed, 32 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/src/channel-display.c b/src/channel-display.c
> > > index 2ea0922..ec94cf8 100644
> > > --- a/src/channel-display.c
> > > +++ b/src/channel-display.c
> > > @@ -63,8 +63,7 @@ struct _SpiceDisplayChannelPrivate {
> > > SpicePaletteCache palette_cache;
> > > SpiceImageSurfaces image_surfaces;
> > > SpiceGlzDecoderWindow *glz_window;
> > > -display_stream **streams;
> > > -int nstreams;
> > > +GHashTable *streams;
> > > gbooleanmark;
> > > guint mark_false_event_id;
> > > GArray *monitors;
> > > @@ -168,7 +167,7 @@ static void spice_display_channel_finalize(GObject
> > > *object)
> > > g_clear_pointer(&c->monitors, g_array_unref);
> > > clear_surfaces(SPICE_CHANNEL(object), FALSE);
> > > g_hash_table_unref(c->surfaces);
> > > -clear_streams(SPICE_CHANNEL(object));
> > > +g_clear_pointer(&c->streams, g_hash_table_unref);
> > > g_clear_pointer(&c->palettes, cache_free);
> > >
> > > if (G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize)
> > > @@ -863,6 +862,7 @@ static void
> > > spice_display_channel_init(SpiceDisplayChannel *channel)
> > > c = channel->priv = SPICE_DISPLAY_CHANNEL_GET_PRIVATE(channel);
> > >
> > > c->surfaces = g_hash_table_new_full(NULL, NULL, NULL,
> > > destroy_surface);
> > > +c->streams = g_hash_table_new_full(g_direct_hash, g_direct_equal,
> > > NULL, display_stream_destroy);
> > > c->image_cache.ops = &image_cache_ops;
> > > c->palette_cache.ops = &palette_cache_ops;
> > > c->image_surfaces.ops = &image_surfaces_ops;
> > > @@ -1207,15 +1207,18 @@ static void report_invalid_stream(SpiceChannel
> > > *channel, uint32_t id)
> > >
> > > static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t
> > > id)
> > > {
> > > +display_stream *st;
> > > SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> > >
> > > -if (c != NULL && c->streams != NULL && id < c->nstreams &&
> > > -c->streams[id] != NULL) {
> > > -return c->streams[id];
> > > +g_return_val_if_fail(c != NULL && c->streams != NULL, NULL);
> > > +
> > > +st = g_hash_table_lookup(c->streams, GUINT_TO_POINTER(id));
> > > +if (st == NULL) {
> > > +spice_printerr("couldn't find stream %u", id);
> > > +report_invalid_stream(channel, id);
> > > }
> > >
> > > -report_invalid_stream(channel, id);
> > > -return NULL;
> > > +return st;
> > > }
> >
> > I haven't studied the code a whole lot, but this looks to be on
> > the hot path of receiving every frame. The complexity is
> > constant in both old and new version, but the hash function has
> > a higher constant. I don't expect the constant to be
> > significant in comparison to the context, but do you have any
> > guesstimations and/or numbers? And do we want to rely on
> > guesstimations or want numbers here?
>
> I don't have numbers as I didn't consider it to be significant at
> the time. My main concern was that it relies on something not
> documented in the protocol itself (stream's ID being a small
> number).
But that is the old code relying on that, right? Any other reason than
that to make this change? (I was thinking it was some preparation for
adding the measurements)
I mean (theoretically! :)) if your patch was how the code was, somebody
could write a patch to change it to what is
Re: [Spice-devel] [spice-gtk v2] channel-display: Use GHashTable to keep stream's structure
Hi,
Thanks for taking a look!
On Mon, Mar 12, 2018 at 04:13:30PM +0100, Lukáš Hrázký wrote:
> On Wed, 2018-03-07 at 09:17 +0100, Victor Toso wrote:
> > From: Victor Toso
> >
> > The major issue with the current approach is that it relies on
> > the ID from SpiceMsgDisplayStreamCreate to create the smallest 2^n
> > sized array that could fit the stream's id as index.
> >
> > This is potentially dangerous as the ID value is not documented and
> > could have any uint32_t value. We depend on Spice server's
> > implementation which currently defines max of 50 streams.
> >
> > > /** Maximum number of streams created by spice-server */
> > > #define NUM_STREAMS 50
> >
> > The first ID has the value of 49* while the c->streams array would be
> > allocated to 64. We could only benefit from allocating on high
> > creating/removal of streams, which is not the case.
> >
> > We can improve the above situations by using a hash table.
> >
> > Signed-off-by: Victor Toso
> > ---
> > Changes from v2:
> > * using g_direct_hash/equal instead of the integer
> > conterpart.
> > * changed the signature from int to uint32_t for id in destroy_stream()
> >
> > src/channel-display.c | 73
> > ++-
> > 1 file changed, 32 insertions(+), 41 deletions(-)
> >
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 2ea0922..ec94cf8 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -63,8 +63,7 @@ struct _SpiceDisplayChannelPrivate {
> > SpicePaletteCache palette_cache;
> > SpiceImageSurfaces image_surfaces;
> > SpiceGlzDecoderWindow *glz_window;
> > -display_stream **streams;
> > -int nstreams;
> > +GHashTable *streams;
> > gbooleanmark;
> > guint mark_false_event_id;
> > GArray *monitors;
> > @@ -168,7 +167,7 @@ static void spice_display_channel_finalize(GObject
> > *object)
> > g_clear_pointer(&c->monitors, g_array_unref);
> > clear_surfaces(SPICE_CHANNEL(object), FALSE);
> > g_hash_table_unref(c->surfaces);
> > -clear_streams(SPICE_CHANNEL(object));
> > +g_clear_pointer(&c->streams, g_hash_table_unref);
> > g_clear_pointer(&c->palettes, cache_free);
> >
> > if (G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize)
> > @@ -863,6 +862,7 @@ static void
> > spice_display_channel_init(SpiceDisplayChannel *channel)
> > c = channel->priv = SPICE_DISPLAY_CHANNEL_GET_PRIVATE(channel);
> >
> > c->surfaces = g_hash_table_new_full(NULL, NULL, NULL, destroy_surface);
> > +c->streams = g_hash_table_new_full(g_direct_hash, g_direct_equal,
> > NULL, display_stream_destroy);
> > c->image_cache.ops = &image_cache_ops;
> > c->palette_cache.ops = &palette_cache_ops;
> > c->image_surfaces.ops = &image_surfaces_ops;
> > @@ -1207,15 +1207,18 @@ static void report_invalid_stream(SpiceChannel
> > *channel, uint32_t id)
> >
> > static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t id)
> > {
> > +display_stream *st;
> > SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> >
> > -if (c != NULL && c->streams != NULL && id < c->nstreams &&
> > -c->streams[id] != NULL) {
> > -return c->streams[id];
> > +g_return_val_if_fail(c != NULL && c->streams != NULL, NULL);
> > +
> > +st = g_hash_table_lookup(c->streams, GUINT_TO_POINTER(id));
> > +if (st == NULL) {
> > +spice_printerr("couldn't find stream %u", id);
> > +report_invalid_stream(channel, id);
> > }
> >
> > -report_invalid_stream(channel, id);
> > -return NULL;
> > +return st;
> > }
>
> I haven't studied the code a whole lot, but this looks to be on
> the hot path of receiving every frame. The complexity is
> constant in both old and new version, but the hash function has
> a higher constant. I don't expect the constant to be
> significant in comparison to the context, but do you have any
> guesstimations and/or numbers? And do we want to rely on
> guesstimations or want numbers here?
I don't have numbers as I didn't consider it to be significant at
the time. My main concern was that it relies on something not
documented in the protocol itself (stream's ID being a small
number).
> Might be a non-issue, just pointing it out as probably the only
> real consideration with this patch?
Evaluating the difference between accessing an array an hash
table seems an overkill to me but, yeah, can be done...
I'm actually touching this part of the code thinking on
measurements but from the decoder's perspective (e.g from
SpiceFrame creation, decoding/rendering and deallocation).
Cheers,
toso
> > /* coroutine context */
> > @@ -1257,45 +1260,36 @@ static display_stream
> > *display_stream_create(SpiceChannel *channel,
> > retu
Re: [Spice-devel] [spice-gtk v2] channel-display: Use GHashTable to keep stream's structure
On Wed, 2018-03-07 at 09:17 +0100, Victor Toso wrote:
> From: Victor Toso
>
> The major issue with the current approach is that it relies on
> the ID from SpiceMsgDisplayStreamCreate to create the smallest 2^n
> sized array that could fit the stream's id as index.
>
> This is potentially dangerous as the ID value is not documented and
> could have any uint32_t value. We depend on Spice server's
> implementation which currently defines max of 50 streams.
>
> > /** Maximum number of streams created by spice-server */
> > #define NUM_STREAMS 50
>
> The first ID has the value of 49* while the c->streams array would be
> allocated to 64. We could only benefit from allocating on high
> creating/removal of streams, which is not the case.
>
> We can improve the above situations by using a hash table.
>
> Signed-off-by: Victor Toso
> ---
> Changes from v2:
> * using g_direct_hash/equal instead of the integer
> conterpart.
> * changed the signature from int to uint32_t for id in destroy_stream()
>
> src/channel-display.c | 73
> ++-
> 1 file changed, 32 insertions(+), 41 deletions(-)
>
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 2ea0922..ec94cf8 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -63,8 +63,7 @@ struct _SpiceDisplayChannelPrivate {
> SpicePaletteCache palette_cache;
> SpiceImageSurfaces image_surfaces;
> SpiceGlzDecoderWindow *glz_window;
> -display_stream **streams;
> -int nstreams;
> +GHashTable *streams;
> gbooleanmark;
> guint mark_false_event_id;
> GArray *monitors;
> @@ -168,7 +167,7 @@ static void spice_display_channel_finalize(GObject
> *object)
> g_clear_pointer(&c->monitors, g_array_unref);
> clear_surfaces(SPICE_CHANNEL(object), FALSE);
> g_hash_table_unref(c->surfaces);
> -clear_streams(SPICE_CHANNEL(object));
> +g_clear_pointer(&c->streams, g_hash_table_unref);
> g_clear_pointer(&c->palettes, cache_free);
>
> if (G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize)
> @@ -863,6 +862,7 @@ static void
> spice_display_channel_init(SpiceDisplayChannel *channel)
> c = channel->priv = SPICE_DISPLAY_CHANNEL_GET_PRIVATE(channel);
>
> c->surfaces = g_hash_table_new_full(NULL, NULL, NULL, destroy_surface);
> +c->streams = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL,
> display_stream_destroy);
> c->image_cache.ops = &image_cache_ops;
> c->palette_cache.ops = &palette_cache_ops;
> c->image_surfaces.ops = &image_surfaces_ops;
> @@ -1207,15 +1207,18 @@ static void report_invalid_stream(SpiceChannel
> *channel, uint32_t id)
>
> static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t id)
> {
> +display_stream *st;
> SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
>
> -if (c != NULL && c->streams != NULL && id < c->nstreams &&
> -c->streams[id] != NULL) {
> -return c->streams[id];
> +g_return_val_if_fail(c != NULL && c->streams != NULL, NULL);
> +
> +st = g_hash_table_lookup(c->streams, GUINT_TO_POINTER(id));
> +if (st == NULL) {
> +spice_printerr("couldn't find stream %u", id);
> +report_invalid_stream(channel, id);
> }
>
> -report_invalid_stream(channel, id);
> -return NULL;
> +return st;
> }
I haven't studied the code a whole lot, but this looks to be on the hot
path of receiving every frame. The complexity is constant in both old
and new version, but the hash function has a higher constant. I don't
expect the constant to be significant in comparison to the context, but
do you have any guesstimations and/or numbers? And do we want to rely
on guesstimations or want numbers here?
Might be a non-issue, just pointing it out as probably the only real
consideration with this patch?
>
> /* coroutine context */
> @@ -1257,45 +1260,36 @@ static display_stream
> *display_stream_create(SpiceChannel *channel,
> return st;
> }
>
> -static void destroy_stream(SpiceChannel *channel, int id)
> +static void destroy_stream(SpiceChannel *channel, uint32_t id)
> {
> SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
>
> g_return_if_fail(c != NULL);
> g_return_if_fail(c->streams != NULL);
> -g_return_if_fail(c->nstreams > id);
>
> -g_clear_pointer(&c->streams[id], display_stream_destroy);
> +g_hash_table_remove(c->streams, GUINT_TO_POINTER(id));
> }
>
> static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn
> *in)
> {
> SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> SpiceMsgDisplayStreamCreate *op = spice_msg_in_parsed(in);
> +display_stream *st;
>
> CHANNEL_DEBUG(channel, "%s: id %u", __FUNC
