This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Consolidate *chr benchtests


On 6 September 2013 09:32, OndÅej BÃlka <neleai@seznam.cz> wrote:
> Hi,
>
> this continue consolidation with *chr benchmarks.
>
> It uses a common bench-chr.h header that i derived from bench-copy.h
> it could be factored more in future.
>
> An additional patch to select representative input combinations would
> be needed.

The description does not mention that things such as test output
change and that the randomisation changes have been added. Again I
feel like it would be much easier to review the changes if the
functional and non-functional changes in the patch were separated.

>  9 files changed, 179 insertions(+), 785 deletions(-)

That is a lot of lines to go through to figure out whether a change is
functional or not.

> OK to commit?
>
>
>         * benchtests/bench-chr.h: New implementation.
>         * benchtests/bench-memchr.c: Include benchtests/bench-chr.h.
>         * benchtests/bench-memrchr.c: Likewise.
>         * benchtests/bench-rawmemchr.c: Likewise.
>         * benchtests/bench-strchr.c: Likewise.
>         * benchtests/bench-strchrnul.c: Likewise.
>         * benchtests/bench-strlen.c: Likewise.
>         * benchtests/bench-strnlen.c: Likewise.
>         * benchtests/bench-strrchr.c: Likewise.
> ---
>  benchtests/bench-chr.h       |  105 ++++++++++++++++++++++++
>  benchtests/bench-memchr.c    |  113 ++------------------------
>  benchtests/bench-memrchr.c   |   28 +++++++
>  benchtests/bench-rawmemchr.c |  110 ++-----------------------
>  benchtests/bench-strchr.c    |  184 ++----------------------------------------
>  benchtests/bench-strchrnul.c |   14 +++-
>  benchtests/bench-strlen.c    |  126 ++---------------------------
>  benchtests/bench-strnlen.c   |  116 ++------------------------
>  benchtests/bench-strrchr.c   |  168 ++------------------------------------
>  9 files changed, 179 insertions(+), 785 deletions(-)
>  create mode 100644 benchtests/bench-chr.h
>  create mode 100644 benchtests/bench-memrchr.c
>
> diff --git a/benchtests/bench-chr.h b/benchtests/bench-chr.h
> new file mode 100644
> index 0000000..7ec6e12
> --- /dev/null
> +++ b/benchtests/bench-chr.h
> @@ -0,0 +1,105 @@
> +/* Measure memcpy, strcpy, stpcpy, ... functions.
> +   Copyright (C) 2013 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#define MEMCPY_RESULT(dst, len) dst
> +#define MIN_PAGE_SIZE (1<<22)

MIN_PAGE_SIZE is not a very descriptive name to me. It does not seem
to be related to machine page size as far as I can tell.

> +#define TEST_MAIN
> +#include "bench-string.h"
> +
> +PROTO
> +IMPLS
> +
> +static void
> +do_one_test (impl_t *impl, size_t aligned, size_t len)
> +{
> +  size_t i, iters = INNER_LOOP_ITERS;
> +  timing_t start, stop, cur;
> +  static unsigned int r_seed = 42;
> +
> +  char *srcs[INNER_LOOP_ITERS];
> +  for (i = 0; i < iters; ++i)
> +    {
> +      srcs[i] = ((char *) buf1) + rand_r (&r_seed)%(MIN_PAGE_SIZE/2);
> +      if (aligned) /* Align pointers. */
> +        {
> +          srcs[i] = srcs[i] - ((uintptr_t)srcs[i])%64;

64 is a magic number, it should be a #define and commented.

> +        }
> +    }
> +
> +  TIMING_NOW (start);
> +  for (i = 0; i < iters; ++i)
> +    {
> +      srcs[i][len] = 0;
> +      CALL2 (impl, srcs[i], len);
> +      srcs[i][len] = 1;
> +    }
> +  TIMING_NOW (stop);
> +
> +  TIMING_DIFF (cur, start, stop);
> +
> +  TIMING_PRINT_MEAN ((double) cur, (double) iters);
> +}
> +
> +static void
> +do_test (size_t aligned, size_t len)
> +{
> +  printf ("Length %4zd, aligned %c:", len, aligned ? 'y' : 'n' );

It is not clear to me what "aligned" "y/n" means, as alignment could
mean anything from 4-byte aligned to page aligned.

The precise value of the alignment is not quite as useful in the
single buffer funciton benchmarks (e.g. strchr, strlen) as it is in
e.g. memcpy but I still think we should try and keep as much
information available as possible.


-- 
Will Newton
Toolchain Working Group, Linaro


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]