[Mesa-dev] [PATCH] mesa/math: Allocate memory for GLmatrix elements and its inverse contiguously

[Thomas Helland]

Hi, and thanks for the patch =)

Have you done any performance testing on this to verify it
gives us a speedup of any kind? I'm asking because it seems like
this might be something that a decent compiler should be able to do.
Performance related patches, at least in core mesa, usually have
some justification with benchmark numbers in the commit message.
Some style comments below

2018-04-17 1:03 GMT+02:00 Vlad Golovkin <vlad.golovkin.mail at gmail.com>:
> When GLmatrix elements and its inverse are stored contiguously in memory it is possible to
> allocate, free and copy these fields with 1 function call instead of 2.
> ---
>  src/mesa/math/m_matrix.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/math/m_matrix.c b/src/mesa/math/m_matrix.c
> index 57a49533de..4ab78a1fb3 100644
> --- a/src/mesa/math/m_matrix.c
> +++ b/src/mesa/math/m_matrix.c
> @@ -1438,8 +1438,7 @@ _math_matrix_is_dirty( const GLmatrix *m )
>  void
>  _math_matrix_copy( GLmatrix *to, const GLmatrix *from )
>  {
> -   memcpy(to->m, from->m, 16 * sizeof(GLfloat));
> -   memcpy(to->inv, from->inv, 16 * sizeof(GLfloat));
> +   memcpy(to->m, from->m, 16 * 2 * sizeof(GLfloat));
>     to->flags = from->flags;
>     to->type = from->type;
>  }
> @@ -1470,12 +1469,17 @@ _math_matrix_loadf( GLmatrix *mat, const GLfloat *m )
>  void
>  _math_matrix_ctr( GLmatrix *m )
>  {
> -   m->m = _mesa_align_malloc( 16 * sizeof(GLfloat), 16 );
> +   m->m = _mesa_align_malloc( 16 * 2 * sizeof(GLfloat), 16 );
>     if (m->m)
> +   {

Our style guides says to keep the curly bracket after an if on the same line.

> +      m->inv = m->m + 16;
>        memcpy( m->m, Identity, sizeof(Identity) );
> -   m->inv = _mesa_align_malloc( 16 * sizeof(GLfloat), 16 );
> -   if (m->inv)
>        memcpy( m->inv, Identity, sizeof(Identity) );
> +   }
> +   else
> +   {

} else {

Although I see that this file defaults to;

{
else {

for some reason. Feel free to follow existing style, or adjust to my comments.
Also, if we want to do this change it deserves a comment in the source.
> +      m->inv = NULL;
> +   }
>     m->type = MATRIX_IDENTITY;
>     m->flags = 0;
>  }
> @@ -1493,7 +1497,6 @@ _math_matrix_dtr( GLmatrix *m )
>     _mesa_align_free( m->m );
>     m->m = NULL;
>
> -   _mesa_align_free( m->inv );
>     m->inv = NULL;
>  }
>
> --
> 2.14.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev