[prev in list] [next in list] [prev in thread] [next in thread] 

List:       postgresql-general
Subject:    Re: [HACKERS] pgbench internal contention
From:       Robert Haas <robertmhaas () gmail ! com>
Date:       2011-07-31 1:46:05
Message-ID: CA+TgmoYsqxdYHSh-NQk+J+HAM3OMxT4Hyazw8XxuvremawwuNQ () mail ! gmail ! com
[Download RAW message or body]

On Sat, Jul 30, 2011 at 9:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> If I'm reading the code right, it only modifies __libc_drand48_data on
> first call, so as long as we called erand48() at least once before
> spawning the child threads, it would probably work.  That seems pretty
> fragile in the face of the fact that they explicitly state that
> they're modifying the global random generator state and that you
> should use erand48_r() if you want reentrant behavior.  So I think if
> we're going to go the erand48() route we probably ought to force
> pgbench to always use our version rather than any OS-supplied version.

Attached is a try at that approach.  Performance benefits are similar
to before.  Same test case as in my OP on this thread, alternating
runs without and with this patch:

tps = 199133.418419 (including connections establishing)
real	5m0.017s
user	23m42.170s
sys	18m46.270s

tps = 226202.289151 (including connections establishing)
real	5m0.018s
user	22m7.520s
sys	9m54.570s

tps = 191144.247489 (including connections establishing)
real	5m0.025s
user	23m35.200s
sys	17m19.070s

tps = 226353.187955 (including connections establishing)
real	5m0.017s
user	21m42.300s
sys	10m9.820s

tps = 189602.248908 (including connections establishing)
real	5m0.044s
user	23m24.060s
sys	17m1.050s

tps = 224521.459164 (including connections establishing)
real	5m0.017s
user	22m9.620s
sys	10m22.590s

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

["pgbench-erand48.patch" (application/octet-stream)]

diff --git a/configure b/configure
index 0e537d9..edae78d 100755
--- a/configure
+++ b/configure
@@ -18923,7 +18923,8 @@ fi
 
 
 
-for ac_func in cbrt dlopen fcvt fdatasync getifaddrs getpeerucred getrlimit memmove \
poll pstat readlink scandir setproctitle setsid sigprocmask symlink sysconf towlower \
utime utimes waitpid wcstombs wcstombs_l +
+for ac_func in cbrt dlopen erand48 fcvt fdatasync getifaddrs getpeerucred getrlimit \
memmove poll pstat readlink scandir setproctitle setsid sigprocmask symlink sysconf \
towlower utime utimes waitpid wcstombs wcstombs_l  do
 as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 { $as_echo "$as_me:$LINENO: checking for $ac_func" >&5
@@ -20493,8 +20494,7 @@ LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e \
's/-lreadline//g'`  
 
 
-
-for ac_func in crypt erand48 getopt getrusage inet_aton random rint srandom strdup \
strerror strlcat strlcpy strtol strtoul +for ac_func in crypt getopt getrusage \
inet_aton random rint srandom strdup strerror strlcat strlcpy strtol strtoul  do
 as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 { $as_echo "$as_me:$LINENO: checking for $ac_func" >&5
diff --git a/configure.in b/configure.in
index 150f9a5..e761aa0 100644
--- a/configure.in
+++ b/configure.in
@@ -1192,7 +1192,7 @@ PGAC_VAR_INT_TIMEZONE
 AC_FUNC_ACCEPT_ARGTYPES
 PGAC_FUNC_GETTIMEOFDAY_1ARG
 
-AC_CHECK_FUNCS([cbrt dlopen fcvt fdatasync getifaddrs getpeerucred getrlimit memmove \
poll pstat readlink scandir setproctitle setsid sigprocmask symlink sysconf towlower \
utime utimes waitpid wcstombs wcstombs_l]) +AC_CHECK_FUNCS([cbrt dlopen erand48 fcvt \
fdatasync getifaddrs getpeerucred getrlimit memmove poll pstat readlink scandir \
setproctitle setsid sigprocmask symlink sysconf towlower utime utimes waitpid \
wcstombs wcstombs_l])  
 AC_REPLACE_FUNCS(fseeko)
 case $host_os in
@@ -1311,7 +1311,7 @@ fi
 pgac_save_LIBS="$LIBS"
 LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
 
-AC_REPLACE_FUNCS([crypt erand48 getopt getrusage inet_aton random rint srandom \
strdup strerror strlcat strlcpy strtol strtoul]) +AC_REPLACE_FUNCS([crypt getopt \
getrusage inet_aton random rint srandom strdup strerror strlcat strlcpy strtol \
strtoul])  
 case $host_os in
 
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index 9d57436..f563a72 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -198,6 +198,7 @@ typedef struct
 	instr_time	start_time;		/* thread start time */
 	instr_time *exec_elapsed;	/* time spent executing cmds (per Command) */
 	int		   *exec_count;		/* number of cmd executions (per Command) */
+	unsigned short random_seed[3];	/* random seed */
 } TState;
 
 #define INVALID_THREAD		((pthread_t) 0)
@@ -380,13 +381,16 @@ usage(const char *progname)
 
 /* random number generator: uniform distribution from min to max inclusive */
 static int
-getrand(int min, int max)
+getrand(TState *thread, int min, int max)
 {
 	/*
 	 * Odd coding is so that min and max have approximately the same chance of
 	 * being selected as do numbers between them.
+	 *
+	 * Our version of erand48 is thread-safe, so we force the use of it
+	 * even if an OS-supplied version is also available.
 	 */
-	return min + (int) (((max - min + 1) * (double) random()) / (MAX_RANDOM_VALUE + \
1.0)); +	return min + (int) (((max - min + 1) * pg_erand48(thread->random_seed)) / \
(MAX_RANDOM_VALUE + 1.0));  }
 
 /* call PQexec() and exit() on failure */
@@ -901,7 +905,7 @@ top:
 		if (commands[st->state] == NULL)
 		{
 			st->state = 0;
-			st->use_file = getrand(0, num_files - 1);
+			st->use_file = getrand(thread, 0, num_files - 1);
 			commands = sql_files[st->use_file];
 		}
 	}
@@ -1068,9 +1072,9 @@ top:
 			}
 
 #ifdef DEBUG
-			printf("min: %d max: %d random: %d\n", min, max, getrand(min, max));
+			printf("min: %d max: %d random: %d\n", min, max, getrand(thread, min, max));
 #endif
-			snprintf(res, sizeof(res), "%d", getrand(min, max));
+			snprintf(res, sizeof(res), "%d", getrand(thread, min, max));
 
 			if (!putVariable(st, argv[0], argv[1], res))
 			{
@@ -2242,6 +2246,9 @@ main(int argc, char **argv)
 		thread->tid = i;
 		thread->state = &state[nclients / nthreads * i];
 		thread->nstate = nclients / nthreads;
+		thread->random_seed[0] = random();
+		thread->random_seed[1] = random();
+		thread->random_seed[2] = random();
 
 		if (is_latencies)
 		{
@@ -2384,7 +2391,7 @@ threadRun(void *arg)
 		Command   **commands = sql_files[st->use_file];
 		int			prev_ecnt = st->ecnt;
 
-		st->use_file = getrand(0, num_files - 1);
+		st->use_file = getrand(thread, 0, num_files - 1);
 		if (!doCustom(thread, st, &result->conn_time, logfile))
 			remains--;			/* I've aborted */
 
diff --git a/src/include/port.h b/src/include/port.h
index 2cab65f..b0676cb 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -380,11 +380,14 @@ extern off_t ftello(FILE *stream);
 #endif
 #endif
 
+extern double pg_erand48(unsigned short xseed[3]);
+extern long pg_lrand48(void);
+extern void pg_srand48(long seed);
+
 #ifndef HAVE_ERAND48
-/* we assume all of these are present or missing together */
-extern double erand48(unsigned short xseed[3]);
-extern long lrand48(void);
-extern void srand48(long seed);
+#define erand48(x) pg_erand48(x)
+#define lrand48() pg_lrand48()
+#define srand48(x) pg_srand48(x)
 #endif
 
 #ifndef HAVE_FSEEKO
diff --git a/src/port/Makefile b/src/port/Makefile
index 60295dc..ffbe95e 100644
--- a/src/port/Makefile
+++ b/src/port/Makefile
@@ -30,8 +30,8 @@ include $(top_builddir)/src/Makefile.global
 override CPPFLAGS := -I$(top_builddir)/src/port -DFRONTEND $(CPPFLAGS)
 LIBS += $(PTHREAD_LIBS)
 
-OBJS = $(LIBOBJS) chklocale.o dirmod.o exec.o inet_net_ntop.o noblock.o \
-	path.o pgcheckdir.o pgmkdirp.o pgsleep.o pgstrcasecmp.o \
+OBJS = $(LIBOBJS) chklocale.o dirmod.o erand48.o exec.o inet_net_ntop.o \
+	noblock.o path.o pgcheckdir.o pgmkdirp.o pgsleep.o pgstrcasecmp.o \
 	qsort.o qsort_arg.o sprompt.o thread.o
 
 # foo_srv.o and foo.o are both built from foo.c, but only foo.o has -DFRONTEND
diff --git a/src/port/erand48.c b/src/port/erand48.c
index 64db7a5..3efbb3f 100644
--- a/src/port/erand48.c
+++ b/src/port/erand48.c
@@ -70,9 +70,9 @@ _dorand48(unsigned short xseed[3])
 	xseed[2] = (unsigned short) accu;
 }
 
-
+/* pgbench relies on this being thread-safe */
 double
-erand48(unsigned short xseed[3])
+pg_erand48(unsigned short xseed[3])
 {
 	_dorand48(xseed);
 	return ldexp((double) xseed[0], -48) +
@@ -81,14 +81,14 @@ erand48(unsigned short xseed[3])
 }
 
 long
-lrand48(void)
+pg_lrand48(void)
 {
 	_dorand48(_rand48_seed);
 	return ((long) _rand48_seed[2] << 15) + ((long) _rand48_seed[1] >> 1);
 }
 
 void
-srand48(long seed)
+pg_srand48(long seed)
 {
 	_rand48_seed[0] = RAND48_SEED_0;
 	_rand48_seed[1] = (unsigned short) seed;



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[prev in list] [next in list] [prev in thread] [next in thread]