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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel