After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 596832 - configurable buffer alignment
configurable buffer alignment
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 0.10.29
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-09-30 08:03 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2010-03-11 08:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
use posix memalign to allocate buffers (3.50 KB, patch)
2009-09-30 08:04 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
use posix memalign to allocate buffers (3.76 KB, patch)
2009-10-02 07:26 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
use posix memalign to allocate buffers (5.51 KB, patch)
2009-10-02 13:03 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
allow using posix memalign to allocate buffers (5.47 KB, patch)
2009-10-05 08:13 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
allow using posix memalign to allocate buffers (6.11 KB, patch)
2010-03-04 08:47 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
allow using posix memalign to allocate buffers (6.12 KB, patch)
2010-03-04 09:02 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-30 08:03:41 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.
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-30 08:04:28 UTC
Created attachment 144359 [details] [review]
use posix memalign to allocate buffers
Comment 2 Tim-Philipp Müller 2009-09-30 08:21:19 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2009-09-30 10:49:12 UTC
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
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-30 12:29:01 UTC
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>
Comment 5 David Schleef 2009-10-01 19:56:09 UTC
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.
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2009-10-02 07:26:18 UTC
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.
Comment 7 Sebastian Dröge (slomo) 2009-10-02 07:36:02 UTC
(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?
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2009-10-02 13:03:17 UTC
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
Comment 9 Jan Schmidt 2009-10-02 13:59:39 UTC
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.
Comment 10 Stefan Sauer (gstreamer, gtkdoc dev) 2009-10-05 08:13:01 UTC
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.
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2010-03-03 13:43:43 UTC
Can I push this after the freeze?
Comment 12 Tim-Philipp Müller 2010-03-03 14:17:16 UTC
It looks like some of the code is duplicated multiple times - maybe that could be factored out into a separate static function?
Comment 13 Stefan Sauer (gstreamer, gtkdoc dev) 2010-03-04 08:47:35 UTC
Created attachment 155212 [details] [review]
allow using posix memalign to allocate buffers

Tim, is that better?
Comment 14 Stefan Sauer (gstreamer, gtkdoc dev) 2010-03-04 09:02:50 UTC
Created attachment 155213 [details] [review]
allow using posix memalign to allocate buffers

After running make check, I decided to go for this patch instead :)
Comment 15 Stefan Sauer (gstreamer, gtkdoc dev) 2010-03-11 08:45:09 UTC
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.