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 v1.1] Randomize memcpy benchmark addresses.


On 4 September 2013 17:50, OndÅej BÃlka <neleai@seznam.cz> wrote:
> I did not pull changes, here is updated version
>
> On Wed, Sep 04, 2013 at 06:31:51PM +0200, OndÅej BÃlka wrote:
>> Hi,
>>
>> This patch randomizes address for memcpy and clears up unnecessary check
>> code in memcpy.
>>
>> This makes results more accurate. We need to calculate averages of these
>> which I leave to Will.

I think it is worthwhile to explain exactly what is being done, why
and any assumptions made.

>> OK to commit?
>>
>         * benchtests/bench-memcpy.c: Randomize address.
>
> diff --git a/benchtests/bench-memcpy.c b/benchtests/bench-memcpy.c
> index 8cd9c23..44deac6 100644
> --- a/benchtests/bench-memcpy.c
> +++ b/benchtests/bench-memcpy.c
> @@ -16,6 +16,10 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>
> +#define SIZE 2000000

This needs a comment and preferably a more descriptive name.

> +#include <stdint.h>
> +
> +
>  #ifndef MEMCPY_RESULT
>  # define MEMCPY_RESULT(dst, len) dst
>  # define MIN_PAGE_SIZE 131072
> @@ -49,32 +53,30 @@ builtin_memcpy (char *dst, const char *src, size_t n)
>  typedef char *(*proto_t) (char *, const char *, size_t);
>
>  static void
> -do_one_test (impl_t *impl, char *dst, const char *src,
> +do_one_test (impl_t *impl, size_t align1, size_t align2,
>              size_t len)
>  {
>    size_t i, iters = INNER_LOOP_ITERS;
>    timing_t start, stop, cur;
> -
> -  if (CALL (impl, dst, src, len) != MEMCPY_RESULT (dst, len))
> +  static unsigned int r_seed = 42;

Isn't randomizing with a static seed value replacing one predictable
sequence with another? And it replaces a sequence that is simple to
reason about with one that is not.

> +  char *srcs[INNER_LOOP_ITERS];
> +  char *dsts[INNER_LOOP_ITERS];
> +  int offset1 = 0, offset2 = 0;
> +  for (int i = 0; i < iters; i++)
>      {
> -      error (0, 0, "Wrong result in function %s %p %p", impl->name,
> -            CALL (impl, dst, src, len), MEMCPY_RESULT (dst, len));
> -      ret = 1;
> -      return;
> +      if (!align1)
> +        offset1 = rand_r (&r_seed) % 64;
> +      srcs[i] = buf1 + (rand_r (&r_seed) % (SIZE / 64)) * 64 + offset1;
> +      if (!align2)
> +        offset2 = rand_r (&r_seed) % 64;
> +      dsts[i] = buf2 + (rand_r (&r_seed) % (SIZE / 64)) * 64 + offset2;

This introduces build warnings:

bench-memcpy.c: In function âdo_one_testâ:
bench-memcpy.c:69:15: warning: pointer targets in assignment differ in
signedness [-Wpointer-sign]
bench-memcpy.c:72:15: warning: pointer targets in assignment differ in
signedness [-Wpointer-sign]

>      }
>
> -  if (memcmp (dst, src, len) != 0)
> -    {
> -      error (0, 0, "Wrong result in function %s dst \"%s\" src \"%s\"",
> -            impl->name, dst, src);
> -      ret = 1;
> -      return;
> -    }

I am not sure how useful this sanity check is. Obviously the glibc
memcpy implementation is tested elsewhere but the simple
implementation isn't.

>    TIMING_NOW (start);
>    for (i = 0; i < iters; ++i)
>      {
> -      CALL (impl, dst, src, len);
> +      CALL (impl, dsts[i], srcs[i], len);
>      }
>    TIMING_NOW (stop);
>
> @@ -86,27 +88,10 @@ do_one_test (impl_t *impl, char *dst, const char *src,
>  static void
>  do_test (size_t align1, size_t align2, size_t len)
>  {
> -  size_t i, j;
> -  char *s1, *s2;
> -
> -  align1 &= 63;
> -  if (align1 + len >= page_size)
> -    return;
> -
> -  align2 &= 63;
> -  if (align2 + len >= page_size)
> -    return;
> -
> -  s1 = (char *) (buf1 + align1);
> -  s2 = (char *) (buf2 + align2);
> -
> -  for (i = 0, j = 1; i < len; i++, j += 23)
> -    s1[i] = j;
> -
> -  printf ("Length %4zd, alignment %2zd/%2zd:", len, align1, align2);
> +  printf ("Length %4zd, aligned %c/%c:", len, align1 ? 'y' : 'n', align2 ? 'y' : 'n');

This means we no longer print what the buffer alignment is which makes
results analysis impossible.

>    FOR_EACH_IMPL (impl, 0)
> -    do_one_test (impl, s2, s1, len);
> +    do_one_test (impl, align1, align2, len);
>
>    putchar ('\n');
>  }
> @@ -117,6 +102,11 @@ test_main (void)
>    size_t i;
>
>    test_init ();
> +  buf1 = malloc (2 * SIZE + 4096);
> +  buf2 = malloc (2 * SIZE + 4096);
> +  buf1 = buf1 + 4096 - ((uintptr_t) buf1) % 4096;
> +  buf2 = buf2 + 4096 - ((uintptr_t) buf1) % 4096;
> +

What is 4096? We should avoid magic constants.

buf1 and buf2 have already been allocated in bench-string.h. We should
make any solution work with that header and work for all string
benchmarks, not just one at a time.

>    printf ("%23s", "");
>    FOR_EACH_IMPL (impl, 0)
> @@ -126,17 +116,13 @@ test_main (void)
>    for (i = 0; i < 18; ++i)
>      {
>        do_test (0, 0, 1 << i);
> -      do_test (i, 0, 1 << i);
> -      do_test (0, i, 1 << i);
> -      do_test (i, i, 1 << i);
> +      do_test (1, 1, 1 << i);
>      }
>
>    for (i = 0; i < 32; ++i)
>      {
>        do_test (0, 0, i);
> -      do_test (i, 0, i);
> -      do_test (0, i, i);
> -      do_test (i, i, i);
> +      do_test (1, 1, i);
>      }
>
>    for (i = 3; i < 32; ++i)
> @@ -144,9 +130,7 @@ test_main (void)
>        if ((i & (i - 1)) == 0)
>         continue;
>        do_test (0, 0, 16 * i);
> -      do_test (i, 0, 16 * i);
> -      do_test (0, i, 16 * i);
> -      do_test (i, i, 16 * i);
> +      do_test (1, 1, 16 * i);
>      }
>
>    do_test (0, 0, getpagesize ());



-- 
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]