[Spice-devel] [spice-gtk v1 04/11] channel-display: use GHashTable to keep stream's structure
Uri Lublin
uril at redhat.com
Tue Apr 10 16:46:46 UTC 2018
On 04/10/2018 02:58 PM, Victor Toso wrote:
> Hi,
>
> On Mon, Apr 09, 2018 at 02:14:15PM +0300, Uri Lublin wrote:
>> On 03/13/2018 01:25 PM, Victor Toso wrote:
>>> From: Victor Toso <me at victortoso.com>
>>>
>>> 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.
>>
>> Some thoughts:
>> 1. It make sense to make it 64 on the server side.
>
> Why? Just so we can alloc a power of two?
Yes, there are 14 entries that are allocated but not used.
It probably does not matter much, as mostly 50 is not reached.
>> 2. Perhaps we should limit the total number of stream allowed
>> (and/or add a message telling the client how many streams
>> the server allocates)
>
> I don't know why we should limit it at this poing.
>
>> 3. O(1) is not exactly 1 and also hash consumes (a little bit)
>> more memory.
>
> Indeed. Hash table takes more time then a direct access in an
> array and uses more memory too but I'm thinking that we are not
> dealing with enough data to consider hash collisions to be a
> problem.
>
>> Other than those and some comments below, it looks good.
>
> Thanks,
>
>>
>>> Signed-off-by: Victor Toso <victortoso at redhat.com>
>>> ---
>>> 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);
>>
>> Is it safer to keep clear_streams similar to clear_surfaces ?
>> There is a difference, clear_surfaces also emits a signal.
>
> The main use for clear_streams() is to clear the streams, which
> does that by calling g_hash_table_remove_all(). In the finalize
> method we are destroying the object so I think g_clear_pointer()
> with unref for the GHashTable works fine.
>
> Maybe I did not understand what you mean with clear_surfaces. I
> did not touch that part but as you said clear_surfaces() is more
> elaborated then clear_streams(). Still, I'd mind to change the
> g_hash_table_unref(c->surfaces) to be a g_clear_pointer(..) too.
What I meant is that the hash table entries are being released only
once anyway, so the overhead is not that big.
I read in g_hash_table_new_full() documentation that sometimes
it's better to call g_hash_table_remove_all before _unref.
However, I think in this case it's not required.
Also I saw that for surfaces too, the entries are freed
before the g_hash_table_unref.
You can leave it as is.
>
>>> 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);
>>
>> - Stream id is an integer, why not keep integer keys
>> (g_int_hash, g_int_equal)?
>> Probably it does not matter much.
>
> It is a unsigned integer. Yes, I was using it in v1 but it wasn't
> working properly while with g_direct_hash() I never had a
> problem. Probably my fault.
>
>> - Why make it different than the line above (using NULL instead of
>> specifying the default functions)
>
> Because the there is not allocation to the hash table's key
> (pointer to uint);
Either I do not understand you or vice versa (or both).
I meant, why not
c->surfaces = g_hash_table_new_full(NULL, NULL, NULL, destroy_surface);
+c->streams = g_hash_table_new_full(NULL, NULL, NULL,
display_stream_destroy);
It does not really matter, just more consistent with current code.
Uri.
> Thanks for the review, let me know if there anything you want me
> to change ;) >
> Cheers,
> toso
>>
>>> 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;
>>> }
>>> /* 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 */
>>>
>>
More information about the Spice-devel
mailing list