Subject: bug#6658: closed (Re: bug#6658: [PATCH] randread: don't require -lrt)

From: [email-masked] (GNU bug Tracking System)
Date: Fri, 23 Jul 2010 23:41:02 +0000

Your bug report

#6658: [PATCH] randread: don't require -lrt

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
6658: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=6658
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#6658: [PATCH] randread: don't require -lrt
From: Pádraig Brady <[email-masked]>
To: Paul Eggert <[email-masked]>
Cc: Jim Meyering <[email-masked]>, [email-masked]
Date: Sat, 24 Jul 2010 00:39:50 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

On 23/07/10 23:20, Paul Eggert wrote:
>>> 3.  We could get about 2X CPU performance on 64-bit machines by using
>>>     ISAAC64 instead of ISAAC.
>>>
>> These are all welcome changes, and the patch for (1) looks fine.
> 
> OK, here's the change for (3), which I just pushed.  I verified that
> it improved performance of random-number generation by 2x on my
> 64-bit host (RHEL 5 host with a Xeon E5620; the benchmark performance
> improvement was actually a tiny bit better than 2x speedup).  On this
> platform isaac_refill now generates pseudorandom data at around 2.5 GB/s,
> which is quite a bit faster than /dev/urandom's 7.5 MB/s
> (as measured with dd).

Very nice.

> On 32-bit hosts the code behaves as before, though I expect it runs a
> bit faster than before (I haven't checked this).

Same performance on 32 bit here:

$ git pull origin
$ gcc -march=pentium-m -O2 -I lib gl/tests/test-rand-isaac.c lib/libcoreutils.a
$ time ./a.out 10000000
real	0m13.093s
$ (cd lib && make)
  CC     randread.o
  CC     rand-isaac.o
  AR     libcoreutils.a
$gcc -march=pentium-m -O2 -I lib gl/tests/test-rand-isaac.c lib/libcoreutils.a
$ time ./a.out 10000000
real	0m13.115s

Note I needed to comment out the ASSERT check
when linking with the old rand-isaac.
Is that a bug in the old implementation or just a change?

> 
> Perhaps this stuff should be moved to gnulib?  I can't recall if there
> was any good reason to leave it in coreutils.
> 

Inertia more than anything I think.

> +++ b/gl/tests/test-rand-isaac.c
> +
> +/* FIXME: once/if in gnulib, use #include "macros.h" in place of this */
> +#define ASSERT(expr) \

Hmm, I just used macros.h in test-mbsalign.c which seems ok

I'm marking this rand update series as done.

cheers,
Pádraig.

Subject: [PATCH] randread: don't require -lrt
From: Paul Eggert <[email-masked]>
To: Bug Coreutils <[email-masked]>
Date: Fri, 16 Jul 2010 14:17:42 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

In looking at the random part of coreutils some more, I see some
issues.

1.  Apps that use random numbers typically must link to -lrt,
    for a very small benefit (nanosecond resolution rather than
    microsecond resolution time stamp for random seed).  Often
    this "benefit" is illusory as the time stamps really are not
    nanosecond resolution.

2.  If /dev/urandom is available, it should be used to seed
    the ISAAC generator.  This will cost more than invoking
    gettimeofday() but it's far more random.

3.  We could get about 2X CPU performance on 64-bit machines by using
    ISAAC64 instead of ISAAC.

The patch below implements (1); I haven't installed it.  I'd like to
do (2) and (3) too, but thought I'd ask for feedback first.

>From aa279ddf6d766ca8f280295632d14cfbf4926c5b Mon Sep 17 00:00:00 2001
From: Paul R. Eggert <[email-masked]>
Date: Fri, 16 Jul 2010 14:02:08 -0700
Subject: [PATCH] randread: don't require -lrt

Programs like 'sort' were linking to -lrt in order to get
clock_gettime, but this was misguided: it wasted considerable
resources while gaining at most 10 bits of entropy.  Almost nobody
needs the entropy, and there are better ways to get much better
entropy for people who do need it.
* gl/lib/rand-isaac.c (isaac_seed): Include <sys/time.h> not
"gethrxtime.h".
(isaac_seed): Use gettimeofday rather than gethrxtime.
* gl/modules/randread (Depends-on): Depend on gettimeofday
and not gethrxtime.
* src/Makefile.am (mktemp_LDADD, shred_LDADD, shuf_LDADD, sort_LDADD):
(tac_LDADD): Omit $(LIB_GETHRXTIME); no longer needed.
---
 gl/lib/rand-isaac.c |    6 +++---
 gl/modules/randread |    2 +-
 src/Makefile.am     |    7 +------
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/gl/lib/rand-isaac.c b/gl/lib/rand-isaac.c
index 52d53a3..377caa6 100644
--- a/gl/lib/rand-isaac.c
+++ b/gl/lib/rand-isaac.c
@@ -35,10 +35,9 @@
 #include "rand-isaac.h"
 
 #include <string.h>
+#include <sys/time.h>
 #include <unistd.h>
 
-#include "gethrxtime.h"
-
 
 /* This index operation is more efficient on many processors */
 #define ind(mm, x) \
@@ -292,7 +291,8 @@ isaac_seed (struct isaac_state *s)
   { gid_t t = getgid ();   ISAAC_SEED (s, t); }
 
   {
-    xtime_t t = gethrxtime ();
+    struct timeval t;
+    gettimeofday (&t, NULL);
     ISAAC_SEED (s, t);
   }
 
diff --git a/gl/modules/randread b/gl/modules/randread
index 9870cc8..efc7958 100644
--- a/gl/modules/randread
+++ b/gl/modules/randread
@@ -11,7 +11,7 @@ Depends-on:
 error
 exitfail
 fopen-safer
-gethrxtime
+gettimeofday
 quotearg
 stdbool
 stdint
diff --git a/src/Makefile.am b/src/Makefile.am
index d87fbb5..1a19fa6 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -328,13 +328,8 @@ ls_LDADD += $(LIB_CLOCK_GETTIME)
 pr_LDADD += $(LIB_CLOCK_GETTIME)
 touch_LDADD += $(LIB_CLOCK_GETTIME)
 
-# for gethrxtime, randint, randread, gen_tempname, mkstemp
+# for gethrxtime
 dd_LDADD += $(LIB_GETHRXTIME)
-mktemp_LDADD += $(LIB_GETHRXTIME)
-shred_LDADD += $(LIB_GETHRXTIME)
-shuf_LDADD += $(LIB_GETHRXTIME)
-sort_LDADD += $(LIB_GETHRXTIME)
-tac_LDADD += $(LIB_GETHRXTIME)
 
 # for cap_get_file
 ls_LDADD += $(LIB_CAP)
-- 
1.7.1