This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v1.1] Randomize memcpy benchmark addresses.
- From: Will Newton <will dot newton at linaro dot org>
- To: Ondřej Bílka <neleai at seznam dot cz>
- Cc: libc-alpha <libc-alpha at sourceware dot org>
- Date: Thu, 5 Sep 2013 08:41:02 +0100
- Subject: Re: [PATCH v1.1] Randomize memcpy benchmark addresses.
- Authentication-results: sourceware.org; auth=none
- References: <20130904163151 dot GB10358 at domone dot kolej dot mff dot cuni dot cz> <20130904165025 dot GA15899 at domone dot kolej dot mff dot cuni dot cz>
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