[Cogl] [PATCH] memory-stack: Make sure the allocations are correctly aligned
Robert Bragg
robert at sixbynine.org
Fri Nov 29 14:58:55 PST 2013
Just as a pedantic coding style thing, I think I'd prefer that for this bit:
@@ -56,7 +56,10 @@ _cogl_magazine_chunk_alloc (CoglMagazine *magazine)
return chunk;
}
else
- return _cogl_memory_stack_alloc (magazine->stack, magazine->chunk_size);
+ /* In this case the alignment doesn't matter because the memory
+ * stack doesn't contain objects with mixed alignment so we can
+ * just use an alignment of 1 */
+ return _cogl_memory_stack_alloc (magazine->stack, magazine->chunk_size, 1);
}
that we put parenthesis around the comment and return statement. It
reminded me of this commit 96d9ea78eb56269c though not nearly as bad
as that.
This looks good to me and I'll nab the same change for Rig too:
Reviewed-by: Robert Bragg <robert at linux.intel.com>
thanks,
Robert
On Fri, Nov 29, 2013 at 7:29 PM, Neil Roberts <neil at linux.intel.com> wrote:
> When allocating an object in the memory stack we should make sure that
> the object is correctly aligned. This adds an alignment parameter to
> _cogl_memory_stack_alloc which is used to add some padding bytes
> after the last allocation as needed.
>
> In order to work out which alignment to use this patch also adds a
> COGL_UTIL_ALIGNOF macro. This will use the standard alignof operator
> from C11 if it's available or it will try __alignof__ which works in
> GCC, or failing that it will just assume an alignment of 8 for
> everything.
>
> Currently this patch is completely academic because the only thing
> that uses the memory stack is the magazine and that always allocates
> objects of the same size in the stack so they will never become
> unaligned. However I think it would be nice to have this patch in
> place in case we start to use the stack allocator for a collection of
> objects with different alignments to avoid mysterious crashes or
> performance problems.
> ---
> cogl/cogl-magazine-private.h | 5 ++++-
> cogl/cogl-memory-stack-private.h | 4 +++-
> cogl/cogl-memory-stack.c | 18 ++++++++++++++----
> cogl/cogl-util.h | 9 +++++++++
> configure.ac | 15 +++++++++++++++
> 5 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/cogl/cogl-magazine-private.h b/cogl/cogl-magazine-private.h
> index c07e0fe..6431765 100644
> --- a/cogl/cogl-magazine-private.h
> +++ b/cogl/cogl-magazine-private.h
> @@ -56,7 +56,10 @@ _cogl_magazine_chunk_alloc (CoglMagazine *magazine)
> return chunk;
> }
> else
> - return _cogl_memory_stack_alloc (magazine->stack, magazine->chunk_size);
> + /* In this case the alignment doesn't matter because the memory
> + * stack doesn't contain objects with mixed alignment so we can
> + * just use an alignment of 1 */
> + return _cogl_memory_stack_alloc (magazine->stack, magazine->chunk_size, 1);
> }
>
> static inline void
> diff --git a/cogl/cogl-memory-stack-private.h b/cogl/cogl-memory-stack-private.h
> index 4e077f2..4d9021a 100644
> --- a/cogl/cogl-memory-stack-private.h
> +++ b/cogl/cogl-memory-stack-private.h
> @@ -32,7 +32,9 @@ CoglMemoryStack *
> _cogl_memory_stack_new (size_t initial_size_bytes);
>
> void *
> -_cogl_memory_stack_alloc (CoglMemoryStack *stack, size_t bytes);
> +_cogl_memory_stack_alloc (CoglMemoryStack *stack,
> + size_t bytes,
> + size_t alignment);
>
> void
> _cogl_memory_stack_rewind (CoglMemoryStack *stack);
> diff --git a/cogl/cogl-memory-stack.c b/cogl/cogl-memory-stack.c
> index 2aefcc9..cb44403 100644
> --- a/cogl/cogl-memory-stack.c
> +++ b/cogl/cogl-memory-stack.c
> @@ -106,17 +106,27 @@ _cogl_memory_stack_new (size_t initial_size_bytes)
> return stack;
> }
>
> +static size_t
> +_cogl_memory_stack_align (size_t base, int alignment)
> +{
> + return (base + alignment - 1) & ~(alignment - 1);
> +}
> +
> void *
> -_cogl_memory_stack_alloc (CoglMemoryStack *stack, size_t bytes)
> +_cogl_memory_stack_alloc (CoglMemoryStack *stack,
> + size_t bytes,
> + size_t alignment)
> {
> CoglMemorySubStack *sub_stack;
> + size_t offset;
> void *ret;
>
> sub_stack = stack->sub_stack;
> - if (G_LIKELY (sub_stack->bytes - stack->sub_stack_offset >= bytes))
> + offset = _cogl_memory_stack_align (stack->sub_stack_offset, alignment);
> + if (G_LIKELY (sub_stack->bytes - offset >= bytes))
> {
> - ret = sub_stack->data + stack->sub_stack_offset;
> - stack->sub_stack_offset += bytes;
> + ret = sub_stack->data + offset;
> + stack->sub_stack_offset = offset + bytes;
> return ret;
> }
>
> diff --git a/cogl/cogl-util.h b/cogl/cogl-util.h
> index b7f705b..ca0e956 100644
> --- a/cogl/cogl-util.h
> +++ b/cogl/cogl-util.h
> @@ -52,6 +52,15 @@
> #define COGL_EXPORT
> #endif
>
> +#if defined(HAVE_ALIGNOF)
> +#define COGL_UTIL_ALIGNOF(x) (alignof (x))
> +#elif defined(HAVE_ALIGNOF_UNDERSCORE)
> +#define COGL_UTIL_ALIGNOF(x) (__alignof__ (x))
> +#else
> +#warning No alignof operator found for this compiler
> +#define COGL_UTIL_ALIGNOF(x) 8
> +#endif
> +
> int
> _cogl_util_next_p2 (int a);
>
> diff --git a/configure.ac b/configure.ac
> index a024234..85251d0 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -158,6 +158,21 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([_Static_assert (1, "");],
> AC_MSG_RESULT([yes])],
> [AC_MSG_RESULT([no])])
>
> +AC_MSG_CHECKING([for alignof])
> +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([const int thing = alignof (int);],
> + [(void) 0])],
> + [AC_DEFINE([HAVE_ALIGNOF], [1],
> + [Whether alignof can be used or not])
> + AC_MSG_RESULT([yes])],
> + [AC_MSG_RESULT([no])])
> +AC_MSG_CHECKING([for __alignof__])
> +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([const int thing = __alignof__ (int);],
> + [(void) 0])],
> + [AC_DEFINE([HAVE_ALIGNOF_UNDERSCORE], [1],
> + [Whether __alignof__ can be used or not])
> + AC_MSG_RESULT([yes])],
> + [AC_MSG_RESULT([no])])
> +
> dnl ================================================================
> dnl Libtool stuff.
> dnl ================================================================
> --
> 1.8.3.1
>
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl
More information about the Cogl
mailing list