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 <me@xxxxxxxxxxxxxx>
> > >
> > > 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 <victortoso@xxxxxxxxxx>
> > > ---
> > > 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;
> > > gboolean mark;
> > > 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 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 :)).
> > 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 :)
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 :)
> 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 :)
Cheers,
Lukas
> Cheers,
> toso
>
> > > /* 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", __FUNCTION__, op->id);
> > > + g_return_if_fail(c->streams != NULL);
> > >
> > > - if (op->id >= c->nstreams) {
> > > - int n = c->nstreams;
> > > - if (!c->nstreams) {
> > > - c->nstreams = 1;
> > > - }
> > > - while (op->id >= c->nstreams) {
> > > - c->nstreams *= 2;
> > > - }
> > > - c->streams = realloc(c->streams, c->nstreams * sizeof(c->streams[0]));
> > > - memset(c->streams + n, 0, (c->nstreams - n) * sizeof(c->streams[0]));
> > > - }
> > > - g_return_if_fail(c->streams[op->id] == NULL);
> > > -
> > > - c->streams[op->id] = display_stream_create(channel, op->id, op->surface_id,
> > > - op->flags, op->codec_type,
> > > - &op->dest, &op->clip);
> > > - if (c->streams[op->id] == NULL) {
> > > + st = display_stream_create(channel, op->id, op->surface_id,
> > > + op->flags, op->codec_type,
> > > + &op->dest, &op->clip);
> > > + if (st == NULL) {
> > > spice_printerr("could not create the %u video stream", op->id);
> > > destroy_stream(channel, op->id);
> > > report_invalid_stream(channel, op->id);
> > > + return;
> > > }
> > > +
> > > + g_hash_table_insert(c->streams, GUINT_TO_POINTER(op->id), st);
> > > }
> > >
> > > static const SpiceRect *stream_get_dest(display_stream *st, SpiceMsgIn *frame_msg)
> > > @@ -1470,18 +1464,21 @@ static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
> > > {
> > > SpiceChannel *channel = data;
> > > SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> > > - guint i;
> > > + GHashTableIter iter;
> > > + gpointer key, value;
> > >
> > > CHANNEL_DEBUG(channel, "%s", __FUNCTION__);
> > >
> > > - for (i = 0; i < c->nstreams; i++) {
> > > - display_stream *st;
> > > + g_hash_table_iter_init (&iter, c->streams);
> > > + while (g_hash_table_iter_next (&iter, &key, &value)) {
> > > + display_stream *st = value;
> > >
> > > - if (c->streams[i] == NULL) {
> > > + if (st == NULL) {
> > > + g_warn_if_reached();
> > > continue;
> > > }
> > > - SPICE_DEBUG("%s: stream-id %u", __FUNCTION__, i);
> > > - st = c->streams[i];
> > > +
> > > + SPICE_DEBUG("%s: stream-id %u", __FUNCTION__, GPOINTER_TO_UINT(key));
> > > st->video_decoder->reschedule(st->video_decoder);
> > > }
> > > }
> > > @@ -1626,13 +1623,7 @@ static void display_stream_destroy(gpointer st_pointer)
> > > static void clear_streams(SpiceChannel *channel)
> > > {
> > > SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> > > - int i;
> > > -
> > > - for (i = 0; i < c->nstreams; i++) {
> > > - destroy_stream(channel, i);
> > > - }
> > > - g_clear_pointer(&c->streams, g_free);
> > > - c->nstreams = 0;
> > > + g_hash_table_remove_all(c->streams);
> > > }
> > >
> > > /* coroutine context */
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel