[Spice-devel] [spice-gtk v2] channel-display: Use GHashTable to keep stream's structure

[Lukáš Hrázký]

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 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!

Allright, thanks for fixing the small things :) I have no practical
experience with glib so I didn't feel like acking anything though :(

Cheers,
Lukas

> Cheers,
>         toso