GNOME Bugzilla – Bug 500150
[filesink] add property to enable buffering via setvbuf()
Last modified: 2007-12-24 19:15:44 UTC
Patch to add call to setvbuf() on opened stream if requested by User. The size of the buffer is set by indicating the number of memory pages.
Created attachment 99761 [details] [review] buffering option
does this patch improve performance in a measurable way?
Yes, we use record over NFS for a large number of files (over 100 simultaneously) and for buffers of 20 milliseconds duration. To avoid too many system call, context switching and so on we use this patch. Some comparisons : [root@testg5 /tmp]$strace -f -v gst-launch audiotestsrc num-buffers=10000 ! filesink location=/tmp/t.dat buffer-size=10 2>&1 | grep write | wc -l 525 [root@testg5 /tmp]$strace -f -v gst-launch audiotestsrc num-buffers=10000 ! filesink location=/tmp/t.dat 2>&1 | grep write | wc -l 5023 For us reducing the number of writes reduces overhead on the nfs client and on the nfs server. We also reduce side effect of context switch on the system.
Doesn't look particularly portable in its current form (getpagesize, __fbufsize).
Right. getpagesize() could be replaced by sysconf() but i don't know if there is a POSIX equivalent to __fbufsize().
It's ok to use these things IMO, as long as you add the necessary checks to configure.ac and make sure you #ifdef stuff as appropriate.
Created attachment 100527 [details] [review] check for stdio_ext.h and getpagesize() function
Created attachment 100528 [details] [review] use autoconf checking to use buffering
done. Buffering only allowed if getpagesize is available.
The buffer size property should be in bytes, not pages. I have a minor preference for encouraging the use of a reblocking element instead of a property on filesink. And, if we decide to add this property, I have a minor preference for making setvbuf() the default, with a reasonable buffer size. Maybe 64kB or so. Please create patches with 'cvs diff -u'. Better yet, add 'diff -u' to your ~/.cvsrc.
Created attachment 100681 [details] [review] configure.ac : check for stdio_ext.h and getpagesize() function
Created attachment 100682 [details] [review] gstfilesink.c : use autoconf checking to use buffering
Disabling buffering can be useful when you want to smooth I/O ( on local or network FS for ex. and working with large buffers) or reduce amount of data lost in case of process failure. So defaulting to setvbuf may not be a good thing (IMHO). Switching from pages to bytes unit is not a problem, this way there is no more need to getpagesize().
Commited with these changes: - buffer-mode can be controlled. This allows for unbuffered, line and fully buffered modes. The default is to not configure any buffering with setvbuf and just leave the defaults (which seems to be 4K buffering for files, line buffering for teminals, unbuffered for stderr). - buffer-size in bytes. There is a default of 64K buffer, enabled when buffer-mode is set to the required kind of buffering. - the check for getpagesize in configure.ac was not needed anymore. This should allow you to configure all options of setvbuf. Based on Patch by: Laurent Glayal <spglegle at yahoo dot fr> * configure.ac: Check for stdio_ext.h for the filesink changes. * plugins/elements/gstfilesink.c: (buffer_mode_get_type), (gst_file_sink_class_init), (gst_file_sink_init), (gst_file_sink_dispose), (gst_file_sink_set_property), (gst_file_sink_get_property), (gst_file_sink_open_file), (gst_file_sink_close_file): * plugins/elements/gstfilesink.h: Add two properties to control the buffering mode and size. API: GstFileSink::buffer-mode API: GstFileSink::buffer-size