xfs
[Top] [All Lists]

Re: [PATCH 04/12] xfsprogs: allow linking against libtcmalloc

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 04/12] xfsprogs: allow linking against libtcmalloc
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 13 Dec 2011 11:05:43 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111202174741.685796560@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111202174619.179530033@xxxxxxxxxxxxxxxxxxxxxx> <20111202174741.685796560@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Dec 02, 2011 at 12:46:23PM -0500, Christoph Hellwig wrote:
> Allow linking against the libtcmalloc library from Google's performance
> tools, which at least for repair reduces the memory usage dramatically.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> Index: xfsprogs-dev/configure.in
> ===================================================================
> --- xfsprogs-dev.orig/configure.in    2011-11-14 13:54:28.000000000 +0000
> +++ xfsprogs-dev/configure.in 2011-11-20 19:21:26.000000000 +0000
> @@ -31,6 +31,26 @@ AC_ARG_ENABLE(editline,
>  AC_SUBST(libeditline)
>  AC_SUBST(enable_editline)
>  
> +AC_ARG_ENABLE(tcmalloc,
> +[ --enable-tcmalloc=[yes/no] Enable tcmalloc [default=no]],,
> +     enable_tcmalloc=check)
> +
> +if test x$enable_tcmalloc != xno; then

Firstly, I don't think this branch of detection code belongs here in
configure.in - m4/package_globals.m4 is where malloc libraries and
compiler flags are set. Indeed, I doubt electric fence is compatible
with tcmalloc, so those options need to be made exclusive....

> +    saved_CPPFLAGS="$CPPFLAGS"
> +    CPPFLAGS="$CPPFLAGS -fno-builtin-malloc"

This needs a comment explaining why this is necessary. Something
like:

"gcc assumes that malloc is implemented by glibc and makes
assumptions and optimisations that break glibc-external
optimisations and malloc-hook debugging. Hence we have to tell it
not to make those optimisations when using tcmalloc."

I also don't see how this "-fno-builtin-malloc" is being passed to
the build environment. There needs to be another variable exported
to add this to the CFLAGS of the build....

> +    AC_CHECK_LIB([tcmalloc_minimal], [malloc], 
> [libtcmalloc="-ltcmalloc_minimal"],
> +        [AC_CHECK_LIB([tcmalloc], [malloc], [libtcmalloc="-ltcmalloc"], [
> +         if test x$enable_tcmalloc = xyes; then
> +            AC_MSG_ERROR([libtcmalloc_minimal or libtcmalloc library not 
> found], 1)
> +         fi]
> +        )]
> +    )

I can't say this is my favourite way of encoding all the options.
I'd prefer something more verbose and explicit that keeps all the
malloc library options in one set of logic. That is, I'm pretty sure
electric fence won't work with tcmalloc, so those options are
multually exclusive. So perhaps putting this in
m4/package_globals.m4:

        tcmalloc=false
        saved_CPPFLAGS="$CPPFLAGS"
        if test $enable_tcmalloc != no ; then
                CPPFLAGS="$CPPFLAGS -fno-builtin-malloc"
                AC_CHECK_LIB(tcmalloc_minimal, malloc, tcmalloc=minimum, 
tcmalloc=false)
                AC_CHECK_LIB(tcmalloc, malloc, tcmalloc=full,)
        fi

        if test $tcmalloc = minimum ; then
                malloc_lib="-ltcmalloc_minimum"
                malloc_cflags="-fno-builtin-malloc"
        fi
        if test $tcmalloc = full ; then
                malloc_lib="-ltcmalloc"
                malloc_cflags="-fno-builtin-malloc"
        fi
        if test $tcmalloc = false ; then
                CPPFLAGS="$saved_CPPFLAGS"
                malloc_cflags=""

                # electric fence malloc debugging might be enabled
                MALLOCLIB=${MALLOCLIB:-''}      # /usr/lib/libefence.a
                malloc_lib="$MALLOCLIB"
        fi

        AC_SUBST(malloc_lib)
        AC_SUBST(malloc_cflags)

And then adding this to include/builddefs.in:

-CFLAGS = @CFLAGS@
+CFLAGS = @CFLAGS@ @malloc_cflags@

would appear to be a better way to do this....

> +    if test x$libtcmalloc = x; then
> +        CPPFLAGS="$saved_CPPFLAGS"
> +    fi
> +fi
> +AC_SUBST(libtcmalloc)
> +

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>