GNOME Bugzilla – Bug 596832
configurable buffer alignment
Last modified: 2010-03-11 08:45:42 UTC
The alignment guaranteed by malloc is not always sufficient. E.g. vector instructions or hardware subsystems want specifically aligned buffers. The attached patch will use posix_memalign if available to allocate buffers. Alignment is pagesize(). We could have a configure option to specify the alignment needs. Elements would still need to check the alignment and do a memcopy if the alignment is not good. E.g. we cannot guarantee alignments when doing subbuffers.
Created attachment 144359 [details] [review] use posix memalign to allocate buffers
Nitpick: please check for the functions you want to use in configure - I don't think HAVE_STDLIB_H implies that posix_memalign() is available.
And one side effect of this is, that you can't override the allocation functions for the buffers anymore (see http://library.gnome.org/devel/glib/unstable/glib-Memory-Allocation.html#g-mem-set-vtable ) because now always the POSIX stuff is used if available. Maybe some embedded systems want some very special allocator
Tim: yes, I was expecting that comment. Will definitely do, just wanted to start the discussion here (you should have seen the patch before:) Slomo: I think the memtable stuff is not so important here. It's mainly used for debugging. Also if you get xvimage buffers or such you cannot override the memory there. So the questions I have: * do we want a configure option for the alignment? default=<some platform specific value> or even pagesize() * alingment=pagesize is wasting quite some bytes, but its still okay for use on embedded (atleast for Nokia) * any issues to make this default if one has posix_memalign, or should it be only used it one configures with --with-aligned-buffers=<n>
Changing alignment from 8 to 16 is only a minor benefit, if any, for most vector systems. Changing alignment from 16 to 32 is very minor, due to cache line alignment. Anything above that has no benefit. I would support making 32 the default alignment, since it does have measurable benefits on existing systems with only minor memory usage. Aligning to page size by default seems to be a bit extreme, especially since the benefits would be limited to certain devices on embedded systems.
Created attachment 144568 [details] [review] use posix memalign to allocate buffers This version of the patch makes the tests pass again. I totally agree that pagesize is overkill for most platforms, 32 bytes alignment seems to be a good default.
(In reply to comment #6) > Created an attachment (id=144568) [details] > use posix memalign to allocate buffers > > This version of the patch makes the tests pass again. I totally agree that > pagesize is overkill for most platforms, 32 bytes alignment seems to be a good > default. That patch still uses getpagesize() ;) IMHO you should make this configurable: --with-buffer-alignment=[0,N,pagesize], default=32 Where 0 would simply fall back to g_malloc, N would be a hardcoded number and pagesize would use the return value of getpagesize(). What do you think?
Created attachment 144593 [details] [review] use posix memalign to allocate buffers This adds a new configure option and use the resulting defines. The configure option: --with-buffer-alignment 8,N,malloc,pagesize (default is 32) The current behavious is: * nothing specified (same as --with-buffer-alignment=malloc) use 8 byte alignment * --with-buffer-alignment (same as --with-buffer-alignment=32) use 32 byte alignment
style pick: Using #if/#elif/#endif is easier to read than #if/#else/#ifdef/#endif/#endif chains. Another side effect of this is that buffers no longer use g_free() by default, which will break the (not common, but does exist) case where elements allocate the buffer data themselves with g_malloc and set the MALLOCDATA() on the buffer. I think GST_BUFFER_FREE_FUNC (buffer) should only be set to free() in gst_buffer_new_and_alloc() and gst_buffer_try_new_and_alloc() - when the buffer allocates the data itself.
Created attachment 144764 [details] [review] allow using posix memalign to allocate buffers Jan, good catch. This patch keeps g_free as a default freefunc and only changes it to free when posiz_memalign is used to allocate data. I also changed the #ifdefs as you recommended.
Can I push this after the freeze?
It looks like some of the code is duplicated multiple times - maybe that could be factored out into a separate static function?
Created attachment 155212 [details] [review] allow using posix memalign to allocate buffers Tim, is that better?
Created attachment 155213 [details] [review] allow using posix memalign to allocate buffers After running make check, I decided to go for this patch instead :)
commit a184419ec5514a9fe804e69cf8bc51eba08b6f84 Author: Stefan Kost <ensonic@users.sf.net> Date: Thu Mar 4 10:44:52 2010 +0200 buffer: allow configurable memory alignment. Fixes #596832 The alignment guaranteed by malloc is not always sufficient. E.g. vector instructions or hardware subsystems want specifically aligned buffers. The attached patch will use posix_memalign if available to allocate buffers. The desired alignment can be set when running configure using the new --with-buffer-alignment option.