GNOME Bugzilla – Bug 767833
souphttpsrc: use dynamic blocksize
Last modified: 2016-06-29 13:02:05 UTC
Allows blocksize to be dynamically adjusted depending on how much is read on the buffers. If the buffer is filled completely, increase the blocksize to read more data in one go. In case only a small portion of the buffer is filled when reading data, reduce the blocksize to avoid wasting memory.
Created attachment 330013 [details] [review] souphttpsrc: dynamically adjust blocksize Update the blocksize depending on how much is obtained from a read of the input stream. This avoids doing too many reads in small chunks when larger amounts of data are available and also prevents using a very large memory area to read a small chunk of data.
Any easy way to measure how this improves performance? Count the number of system calls? Measure the average of the blocksize? Also, do we want to make this optional in case users would want a fixed blocksize?
Maybe time souphttpsrc location=http://foobar.com/largefile.mkv ! fakesink for starters? The effects might be more pronounced on embedded systems though. Number of syscalls is interesting. Number of buffers pushed as well, if you have a high-bitrate stream and push everything in 4096 buffers that's just inefficient and silly.
I don't know how important it is to support a fixed blocksize. Did souphttpsrc guarantee that if you set blocksize=N you'd always get exactly N bytes before? What would be nice to support though is to set a initial target blocksize, so hlsdemux can set something sensible based on the bitrate and fragment duration for example, then you don't have to wait until it ratches up the block size at the beginning. I guess that means that perhaps one should make the adaptive strategy thing a new property of sorts (boolean, or enum for future extension?).
(In reply to Tim-Philipp Müller from comment #4) > I don't know how important it is to support a fixed blocksize. Did > souphttpsrc guarantee that if you set blocksize=N you'd always get exactly N > bytes before? I don't think so, it seems to always give you as much as you currently have? I remember seeing different sized buffers from souphttpsrc
Times for downloading a 1GB file, before and after the patch: real 8m16.712s real 5m49.651s Used strace to track systemcalls but didn't show any difference, perhaps I made a mistake on measuring it. Anyway the times above for downloading shows a good improvement. On the implementation, we didn't guarantee the blocksize in the previous release. Do we add the enum now or leave it to when we find a real use case?
That's a nice improvement! (Would also be interesting to compare to 1.8 souphttpsrc with 1.9 with adaptive buffersize) I'd say let's get it in and not bother with a property for now. Should also test somehow that the downwards adjustment also works.
Times are compatible with 1.8. Got around 6min45s here for both 1.8 and with this patch in a few runs right now. I also confirmed that the adjustment is also working when reducing the blocksize. Any further concerns or shall I merge?
Go for it \o/
Merge please :) For some reason I was under the impression that using the new API would be more efficient as well (i.e. I expected it to be faster than 1.8). Ah well, if it just simplified the code that's just as well I guess ;)
commit 11c61f14bf98946476e3dfe31e7af895604cbcbe Author: Thiago Santos <thiagossantos@gmail.com> Date: Sun Jun 19 02:08:25 2016 -0300 souphttpsrc: dynamically adjust blocksize Update the blocksize depending on how much is obtained from a read of the input stream. This avoids doing too many reads in small chunks when larger amounts of data are available and also prevents using a very large memory area to read a small chunk of data. https://bugzilla.gnome.org/show_bug.cgi?id=767833